kit: Bundle kit in build step

[GrygrFlzr]

Closes #324.

This tells vite to always bundle @sveltejs/kit during the build step, so that @sveltejs/kit is not required in production. This is especially important for serverless functions, which do not allow importing such modules.

TODO: Does this also need to be done for other client dependencies? Yes, it does.

[GrygrFlzr]

Not entirely sure why pnpm -r build failed on the Ubuntu CI, it ran succesfully on my local Windows machine. The error message mentions Transforming async generator functions to the configured target environment is not supported yet.

[Conduitry]

I'm seeing the same error when building locally on Ubuntu. Maybe on Windows the noExternal: ['@sveltejs/kit'] isn't actually doing anything because of a slash direction issue?

[GrygrFlzr]

It's definitely bundling kit into app.js in my local run. Is it a Node 12 vs 14 thing? I'm on Node v14.16.0.

That said, it was good to re-check the built app.js files, because I just noticed it didn't bundle node-fetch for the realworld example. Should we also be reading prod dependencies from package.json and bundling those, or should that be left to the user to configure in vite.config.js?

[babichjacob]

From what I've learned from 10 minutes of experimenting, node-fetch can't be bundled, which led to this error

[GrygrFlzr]

Specifically, it's failing on this code inside node-fetch:

/**
 * @param {FormData} form
 * @param {string} boundary
 */
export async function * formDataIterator(form, boundary) {
	for (const [name, value] of form) {
		yield getHeader(boundary, name, value);

		if (isBlob(value)) {
			yield * value.stream();
		} else {
			yield value;
		}

		yield carriage;
	}

	yield getFooter(boundary);
}

...except, again, not on Windows + Node 14, where it bundles it fine???

[babichjacob]

Specifically, it's failing on this code inside node-fetch:

/**
 * @param {FormData} form
 * @param {string} boundary
 */
export async function * formDataIterator(form, boundary) {
	for (const [name, value] of form) {
		yield getHeader(boundary, name, value);

		if (isBlob(value)) {
			yield * value.stream();
		} else {
			yield value;
		}

		yield carriage;
	}

	yield getFooter(boundary);
}

...except, again, not on Windows + Node 14, where it bundles it fine???

(Genuine question because this isn't something I know) how do you know it's bundling successfully rather than not even attempting to bundle (like Conduitry may have suggested)?

[GrygrFlzr]

After running pnpm -r build, I check the build artifacts (e.g. examples/sandbox/build/app.js) and I can see:

• The only mentions of node-fetch inside are for the HTTP headers of making the request.
• It no longer references @sveltejs/kit and now starts with:

var __assign = Object.assign;
import {createHash} from "crypto";
import Stream from "stream";
import http from "http";
import Url, {parse, resolve, URLSearchParams} from "url";
import https from "https";
import zlib from "zlib";

Compare to the main branch, which does not have the changes to bundle kit:

import {ssr} from "@sveltejs/kit/ssr";
function run(fn) {
  return fn();
}
function blank_object() {
  return Object.create(null);
}
function run_all(fns) {
  fns.forEach(run);
}

[Conduitry]

On Ubuntu, I'm seeing the same error on Node 12, 14, and 15, so it doesn't seem to be related to that.

[babichjacob]

Depending on your definition of work, it works again to put

			external: ['node-fetch'],

before this line:

kit/packages/kit/src/core/build/index.js

Line 392 in 5005899

noExternal: ['svelte', '@sveltejs/kit']

and move "node-fetch": "^3.0.0-beta.8", from being a devDependency to a regular dependency in @sveltejs/kit.

[babichjacob]

This makes the beginning of my built app.js file in sandbox:

var __assign = Object.assign;
import {createHash} from "crypto";
import fetch, {Response} from "node-fetch";
import {parse, resolve, URLSearchParams} from "url";
var chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_$";
var unsafeChars = /[<>\b\f\n\r\t\0\u2028\u2029]/g;
var reserved = /^(?:do|if|in|for|int|let|new|try|var|byte|case|char|else|enum|goto|long|this|void|with|await|break|catch|class|const|final|float|short|super|throw|while|yield|delete|double|export|import|native|return|switch|throws|typeof|boolean|default|extends|finally|package|private|abstract|continue|debugger|function|volatile|interface|protected|transient|implements|instanceof|synchronized)$/;
var escaped$1 = {
  "<": "\\u003C",
  ">": "\\u003E",
  "/": "\\u002F",
  "\\": "\\\\",
  "\b": "\\b",
  "\f": "\\f",
  "\n": "\\n",
  "\r": "\\r",
  "	": "\\t",
  "\0": "\\0",
  "\u2028": "\\u2028",
  "\u2029": "\\u2029"
};

Which isn't 100% the same as the one on Windows. Is Vite falling back to Rollup instead of esbuild on Windows?

[Conduitry]

I just tried this on Window on Node 14, and I'm seeing this same error about transforming for-await loops as I'm seeing on Ubuntu and as CI is seeing.

[GrygrFlzr]

Oooooh, that's why - I hadn't run pnpm i for several days, so I was still using the non-beta build of node-fetch in my dependencies. I can replicate this locally now.

This places us in a pickle though, cause externalizing node-fetch means that it can't be used in serverless environments where importing non-node modules is not an option (whether prod or dev dependencies).

[Conduitry]

Updating node-fetch had happened in #467 and was needed to fix a couple of other things I'm not clear on.

[GrygrFlzr]

The change was made in order to support Node 14.0.0 - 14.12.0, whereas we would have needed Node 12.17.0+ / Node 14.13.0+ minimum if we used the stable version of node-fetch. Kind of a strange workaround though.

@benmccann @Rich-Harris Thoughts? Which is more important at the moment, getting serverless adapters to work, or supporting Node 14.0.0 - 14.12.0? I haven't actually tested if the main branch even builds on these older versions. I have no idea how long it would take for esbuild to support async generator functions.
EDIT: Supported for es2018+

[GrygrFlzr]

I'm an idiot.

https://github.com/evanw/esbuild/blob/b1a394b4b9d83a3bcbe64de27b7d5c45881e5d15/CHANGELOG.md#041

Async generator functions require --target=es2018
This fixes a bug where async generator functions were incorrectly allowed with --target=es2017, which is incorrect because the asynchronous iteration spec is part of ES2018.

It does support it. We just need to target es2018+.

Not sure if that's a better or worse tradeoff than dropping support for Node 14.0.0 - 14.12.0, but I guess I'll try that.

[babichjacob]

I'm an idiot.

https://github.com/evanw/esbuild/blob/b1a394b4b9d83a3bcbe64de27b7d5c45881e5d15/CHANGELOG.md#041

Async generator functions require --target=es2018
This fixes a bug where async generator functions were incorrectly allowed with --target=es2017, which is incorrect because the asynchronous iteration spec is part of ES2018.

It does support it. We just need to target es2018+.

Not sure if that's a better or worse tradeoff than dropping support for Node 14.0.0 - 14.12.0, but I guess I'll try that.

Better. This is server-side code and Node.js has supported async iteration since 10.0.0 and async generators since... uh... I couldn't find an answer for that one. Certainly they'd have to be added in the same version, right?

[Conduitry]

https://node.green/#ES2018-features-Asynchronous-Iterators-async-generators says that async generators and async iterators were added at the same time, in Node 10.

Looking at that chart, the only ES2018 feature not supported in 10.0.0 is https://node.green/#ES2018-misc--Proxy--ownKeys--handler--duplicate-keys-for-non-extensible-targets so I think we're safe targeting that.

[GrygrFlzr]

Okay, I've modified all vite.config.js to bundle production dependencies like node-fetch for realworld. This allows the user to opt out should they need to do it for some particular reason, since it wouldn't be hardcoded into svelte-kit build. I think @sveltejs/kit probably should still be force-bundled though.