Make build folder of adapter-node self-sufficient

[tylkomat]

Currently you can't just take the content of the build folder to run the server in production.
The server won't start without an existing package.json file which is configured with type: "module".

This PR creates a package.json file in the build folder.

To avoid errors after starting the server because if dependencies which should be in dependencies instead of devDependencies of package,json, this PR also extends the build process to create a production ready package.json file.

Can't you just use the project's package.json and lockfile? Why do you need to generate a new one?
I believe the compiler is smarter than I am, so I let it decide which packages should be dependencies in a production package.json.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

⚠️ No Changeset found

Latest commit: ac81974

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Rich-Harris]

Thank you. This is very adapter-node specific, so I don't think we need to add a new generatePackageJson API, the logic should be contained within that adapter.

Having said that, this makes me nervous. Putting a generated package.json in the output folder without a lockfile means that the production build will end up using different versions of packages, which is a non-starter. But you can't just copy the lockfile, because it won't match the generated package.json.

I think it's better if people are responsible for including/excluding files themselves.

[tylkomat]

One option could be to fix the currently installed versions in the generated package.json. Then there are no surprises. It will be the same versions installed in production that you had when you developed it.

Otherwise I would argue, that just copying the package.json from the project is the same as letting people include/exclude files themselves, since this is what most of them (from my experiences on the discord) do (at least they keep the next version of sveltekit). They copy the package.json and then get surprised if everything explodes on the next update. At least it is consistent.

I personally fix the versions in package.json, even in during development, to not get any surprises. After learning the hard way when a project completely broke after touching it one year later.

I only use adapter-node and wasn't sure if it would be interesting for other adapters too. If this PR has a chance of getting merged, I'll move generatePackageJson to the adapter.

[benmccann]

Can't you just use the project's package.json and lockfile? Why do you need to generate a new one? (The answers to questions like these would be good to include in the PR description and code comments)

[tylkomat]

I believe the compiler is smarter than I am, so I let it decide which packages should be dependencies in a production package.json.

[Rich-Harris]

One option could be to fix the currently installed versions in the generated package.json. Then there are no surprises. It will be the same versions installed in production that you had when you developed it.

This works fine for your immediate dependencies. It doesn't work for their dependencies — that's the problem lockfiles solve.

Can't you just use the project's package.json and lockfile?

This seems like a good approach to me. I'm not sure if there are any drawbacks — obviously the file will include stuff that doesn't make any sense (like a "dev" script, etc) but if it's only being used to determine which dependencies to install then that's probably fine.

I believe the compiler is smarter than I am, so I let it decide which packages should be dependencies in a production package.json.

You'd be surprised! Giving users the ability to control which dependencies are included in a production build (by putting them in pkg.dependencies instead of pkg.devDependencies) is easier to understand, more consistent with the logic that determines what stuff gets bundled, and offers an escape hatch for when someone inevitably hits a bizarre edge case.

[Conduitry]

Using the same package.json and lockfile sounds like the right path to me. Sure it'll have stuff in it you don't need, but those will be just lines in a text file. You should be installing dependencies with npm ci --prod when you're preparing your prod build. If you're using Docker, for example, you should be using a multi stage build so that you install all dependencies to build the app and then just install prod dependencies in the final stage and copy over the contents of build.

[tylkomat]

One option could be to fix the currently installed versions in the generated package.json. Then there are no surprises. It will be the same versions installed in production that you had when you developed it.

This works fine for your immediate dependencies. It doesn't work for their dependencies — that's the problem lockfiles solve.

true

Can't you just use the project's package.json and lockfile?

This seems like a good approach to me. I'm not sure if there are any drawbacks — obviously the file will include stuff that doesn't make any sense (like a "dev" script, etc) but if it's only being used to determine which dependencies to install then that's probably fine.

I believe the compiler is smarter than I am, so I let it decide which packages should be dependencies in a production package.json.

You'd be surprised! Giving users the ability to control which dependencies are included in a production build (by putting them in pkg.dependencies instead of pkg.devDependencies) is easier to understand, more consistent with the logic that determines what stuff gets bundled, and offers an escape hatch for when someone inevitably hits a bizarre edge case.

If you don't know how to decide which goes where you build your app and try to run it. You hit the first missing dependency, you update package.json, then try again. Rinse and repeat. In my project I had 7 such dependencies. Then you copy everything to the server and try to run it. Then you hit the missing package.json issue. This is very frustrating.

How about making it optional via adapter configuaration parameter.
Another approach could be svelte-kit sync or svelte-check to hint that some packages should be moved.

[Rich-Harris]

I don't understand how you're encountering 'missing' dependencies — either they're prod dependencies, or they'll be bundled with esbuild. Can you elaborate?

[tylkomat]

The reason why I generated the package.json file comes from the question which packages go where? Is a specific package a devDependency or is it a prod dependency? How do you decide? This question was asked a lot of times on discord. If you don't know you may put everything in devDependencies and sort it out when you want to deploy.

If you don't know what to look for after building you may end up in the rinse and repeat cycle described earlier. Since you put everything into devDependencies (or you missed something in dependencies), some dependencies may not have been bundled (I also did an issue earlier #5849) .

If you know what to look for you can build once and then search for imports and find those which don't import from a relative path. Relative path import means the dependency has been bundled. Some dependencies can't be bundled so they are imported from node_modules.

Again, how do you decide which package is a prod dependency? One Answer was: "When you need it in production it is a prod dependency.". May work, but may not be completely true. You may not miss any packages, but you may add more packages into prod dependencies then necessary. Which is fine I guess.

From my observations everything that is not treeshakable is not bundled. So if it's esm, it is usually bundled.

But why should I spent my time on figuring that out? The compiler does the bundling, so it knows what can be bundled and what can't. I let it decide and generate the package.json accordingly.

[Rich-Harris]

I'm sorry, I still don't follow. The logic is very simple: if something is a prod dependency, it will be excluded from the generated bundle, and must be installed alongside your deployment:

kit/packages/adapter-node/index.js

Lines 52 to 62 in 9dbcce9

	await esbuild.build({
	platform: 'node',
	sourcemap: 'linked',
	target: 'es2022',
	entryPoints: [`${tmp}/index.js`, `${tmp}/manifest.js`],
	outdir: `${out}/server`,
	splitting: true,
	format: 'esm',
	bundle: true,
	external: [...Object.keys(pkg.dependencies || {})]
	});

This is described in the README: https://github.com/sveltejs/kit/tree/master/packages/adapter-node#deploying

some dependencies may not have been bundled

In what circumstance? If that's happening, it's a very strange bug and it needs a reproduction.

[tylkomat]

The problem I'm trying to solve starts earlier.
Let me give you an example. These are your dependencies. Which one is a prod dependency and which one is devDependency?

"@felte/reporter-svelte": "1.1.2",
"@felte/validator-vest": "1.0.9",
"@playwright/test": "1.25.0",
"@prisma/client": "4.2.1",
"@sveltejs/adapter-node": "1.0.0-next.89",
"@sveltejs/kit": "1.0.0-next.480",
"@tailwindcss/forms": "0.5.2",
"@tailwindcss/typography": "0.5.4",
"@types/nodemailer": "6.4.5",
"@typescript-eslint/eslint-plugin": "5.33.0",
"@typescript-eslint/parser": "5.33.0",
"autoprefixer": "10.4.8",
"eslint": "8.21.0",
"eslint-config-prettier": "8.5.0",
"eslint-plugin-svelte3": "4.0.0",
"felte": "1.2.2",
"form-data": "4.0.0",
"got": "12.3.1",
"mdsvex": "0.10.6",
"nodemailer": "6.7.7",
"paseto": "3.1.0",
"postcss": "8.4.16",
"postcss-load-config": "4.0.1",
"prettier": "2.7.1",
"prettier-plugin-svelte": "2.7.0",
"promise-retry": "2.0.1",
"soap": "0.45.0",
"svelte": "3.49.0",
"svelte-check": "2.8.0",
"svelte-preprocess": "4.10.7",
"tailwindcss": "3.1.8",
"tslib": "2.4.0",
"typescript": "4.7.4",
"vest": "4.6.2",
"vite": "3.1.0"

[benmccann]

@tylkomat just make everything a dev dependency and be done with it

[tylkomat]

@benmccann This is exactly what I'm trying to explain. If you do that you'll end up in the rinse and repeat cycle I described earlier. At least you did before #6301 was merged. Nonetheless you'll end up with 7 dependencies that are not bundled.

I created a stripped down example of the problem here, which just includes 3 of the dependencies.
It works fine in dev mode. When you build it the server starts fine.

Before #6301 the server wouldn't start because dependencies where missing (because they couldn't be bundled), I also can provide an example for that.
Now the server starts fine, but when you access the page it explodes, which is even worse since you now expect everything to be fine. Also the error doesn't help you much to identify what the issue could be.

$ node ../no-bundling-prod/
Listening on 0.0.0.0:3000
Error: Dynamic require of "events" is not supported
    at file:///.../no-bundling-prod/server/chunk-BHN6OJC3.js:12:9
    at node_modules/cacheable-request/src/index.js (file:///.../no-bundling-prod/server/2-VOWIZDTZ.js:1345:25)
    at __require2 (file:///.../no-bundling-prod/server/chunk-BHN6OJC3.js:15:50)
    at file:///.../no-bundling-prod/server/2-VOWIZDTZ.js:15405:40
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:541:24)
    at async Promise.all (index 1)
    at async render_page (file:///.../no-bundling-prod/server/index.js:2032:19)
    at async resolve (file:///.../no-bundling-prod/server/index.js:2520:22)

The server is build as a sibling to the source folder to simulate copying the content of the build folder to a server.
In a second step you have to copy the package.json to the build folder (../no-bundling-prod), but don't run npm install, since we're expecting the dependencies to be bundled.

If you try it with this PR the 3 dependencies will be identified in the generated package.json file and you don't have any problems.

@Rich-Harris If you get nervous about generating a package.json file. That is fine. Just some message or build error telling the developer: Hey you have some dependencies in your code that will cause you a serious headache when you don't move them to dependencies; has a similar weight.

EDIT: I tried removing packages. When I remove got, which probably created the error above I get an error from the form-data package.

$ node ../no-bundling-prod/
Listening on 0.0.0.0:3000
Error: Dynamic require of "util" is not supported
    at file:///C:/Users/tylkomat/Documents/lup/no-bundling-prod/server/chunk-BHN6OJC3.js:12:9
    at node_modules/combined-stream/lib/combined_stream.js (file:///C:/Users/tylkomat/Documents/lup/no-bundling-prod/server/2-XV7BKZWM.j
s:105:16)
    at __require2 (file:///C:/Users/tylkomat/Documents/lup/no-bundling-prod/server/chunk-BHN6OJC3.js:15:50)
    at node_modules/form-data/lib/form_data.js (file:///C:/Users/tylkomat/Documents/lup/no-bundling-prod/server/2-XV7BKZWM.js:9120:26)
    at __require2 (file:///C:/Users/tylkomat/Documents/lup/no-bundling-prod/server/chunk-BHN6OJC3.js:15:50)
    at file:///C:/Users/tylkomat/Documents/lup/no-bundling-prod/server/2-XV7BKZWM.js:10650:32
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:541:24)
    at async Promise.all (index 1)

It runs fine when I remove form-data as well. Looks like vest is bundled by esbuild.
The other two packages access builtin NodeJS modules (util, event?).

[Rich-Harris]

The issue here isn't that dependencies aren't being bundled, it's that dependencies that shouldn't be bundled (because they're not compatible with esbuild in esm mode) are being bundled. Ultimately this is a bug in esbuild (evanw/esbuild#1921) — there's no reason why require('util') should be turned into __require('util'), given that it's a built-in module — but it's something we have to deal with for now.

This PR (if we ignore the complexity around lockfiles etc) would change that by making everything external by default. But that's undesirable since it increases the size of deployments.

I think the best solution might be to work around evanw/esbuild#1921, perhaps by generating CommonJS output instead of ESM. I've opened a PR that implements that idea: #6855

[tylkomat]

Thanks @Rich-Harris. Looks like #6896 solved it.