always bundle devDependencies

[Rich-Harris]

Closes #3176.

This is really just an idea, I'd be curious to know if people think it's the right one. It causes devDependencies (or rather, everything that isn't explicitly marked as a prod dependency) to be bundled in the Vite output, which makes the result more portable — you don't need to install devDependencies to run your app. If everything is in devDependencies then it means your app is completely self-contained and you don't need to npm install anything to run it.

Since we use esbuild inside certain adapters (the ones where using node_modules isn't an option, like Cloudflare Workers) this change really only affects users of adapter-node, and we could therefore argue that it doesn't belong in the default config. It also feels a bit messy putting it here. But aside from making builds very slightly slower (albeit in a way that's completely in users' control, just by moving things between dependencies and devDependencies) it seems like the most pragmatic solution.

I also considered adding this logic to the Vite plugin so that it didn't need to be added to the user config, but that felt a bit aggressive.

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]

🦋 Changeset detected

Latest commit: 0545345

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
create-svelte	Patch

Not sure what this means? Click here to learn what changesets are.

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

[Conduitry]

This configuration caused problems for me in dev mode with devdeps that were only in CJS, thus the command check in my snippet. I'll try to put together an example when I'm in front of a computer.

[Rich-Harris]

Blurgh. Is that an argument for tucking this away in the plugin, where we can control that without making everyone's configs more complicated, or is that a bad idea?

[Conduitry]

I'd probably be in favor of hiding this in the plugin, if we could figure out what sort of merging we wanted to do with the user's explicit config. Maybe only include it if they don't have an ssr key at all in their config?

---

A library that is causing me problems with this is https://www.npmjs.com/package/accept-language-parser - I'm trying to use this in server code, and in dev mode with the proposed Vite config I'm getting the error

ReferenceError: module is not defined
    at eval (/node_modules/accept-language-parser/index.js:75:1)
    at instantiateModule (file:///.../node_modules/vite/dist/node/chunks/dep-0fc8e132.js:50548:15)

in the browser, which certainly makes it sound like Vite is trying to read the CJS module in the package as-is as an ESM module. I haven't spent any time figuring out what Vite's doing here, or what Vite should be doing here, or whether there's already an issue for this.

[Conduitry]

Have you looked at the test failures? That they are mostly about $env being imported in places where it shouldn't concerns me, given exactly what's being changed in this PR. I'm also not using $env in the app where I'm successfully using this Vite config, so I don't really have any first-hand knowledge of whether this actually works.

[Rich-Harris]

The $env stuff is a red herring — it's just noisy stderr, not indicative of an actual problem (though it might be nice to get it cleaned up). The real issue AFAICT was something much stranger. Not sure why it suddenly surfaced now

[Rich-Harris]

Maybe only include it if they don't have an ssr key at all in their config?

Unfortunately I don't think this is possible — by the time the config hook in the SvelteKit Vite plugin gets called, a bunch of stuff has already been added to it (by vite-plugin-svelte, AFAICT, though presumably the same thing would apply with whatever plugins the user added). So we either have to rely on Vite's config merging logic, or ask users to have a more complex vite.config.js that exports a function.

[benmccann]

Yeah, Vite dev bundling is a hot mess, so I definitely would not enable it in dev, but would follow @Conduitry's example.

This PR feels like it's probably the best compromise to me. The only thing I don't love about it is that it makes the default template more complicated, but I could get on board with that

I'd be quite hesitant to make it part of the plugin because then the default Vite config is different in ways that the user might not expect and are hard to override even if they do. E.g. if they want to bundle something that we've added to external I believe they would be out-of-luck

[dummdidumm]

Some people in the issue said this didn't work for them - does anybody know more about why that is?

[tylkomat]

Maybe this can be combined with the PR I planed to do: https://github.com/tylkomat/kit/tree/package_json

The changes would generate a production package.json file which only includes the dependencies which are needed for production and couldn't be bundled.

This would solve the mentioned problems in #3176 that the result has to be run to figure out which packages should have been moved from devDeps to deps.

[dominikg]

vite-plugin-svelte does some automated processing to handle dependencies that contain .svelte files or have svelte as a dependency.

https://github.com/sveltejs/vite-plugin-svelte/blob/main/packages/vite-plugin-svelte/src/utils/options.ts#L401

This is required to be able to resolve svelte with svelte/ssr in vite-plugin-svelte which turns onMount into a noop.

If someone were to put such a dependency in dependencies and it ended up in ssr.external, that would break.

[AlexRMU]

Errors appear:

[commonjs--resolver] Cannot bundle Node.js built-in "..." imported from "...". Consider disabling ssr.noExternal or remove the built-in dependency.
[vite-plugin-svelte-kit] Cannot bundle Node.js built-in "..." imported from "...". Consider disabling ssr.noExternal or remove the built-in dependency.

Solution:

import { builtinModules } from "module";
...
{
    build: {
            rollupOptions: {
                external: builtinModules,
            },
    }
}

[AlexRMU]

And look at Automattic/mongoose#12335

[benmccann]

@tylkomat's solution is interesting: tylkomat@1a57072

[Rich-Harris]

Not sure I understand — looks like that creates a package.json with a dependencies that includes the things that didn't get bundled, but it doesn't affect whether or not they did get bundled which is the issue here, no?

Feels like adding an additional package.json rather than relying on well-understood dev vs prod semantics would create additional complexity, unless I've misunderstood?

[tylkomat]

Yes you understood correctly.
I wanted to solve the problem of what has to be in dependencies and what is fine in devDependencies. Easier developer journey, just put everything into devDependencies and the compiler will sort it out. In the end you get a build folder with all the content inside that you need to run the application. (A package.json with type: "module" is needed anyway otherwise server won't start).

The question is if you can safely bundle every piece of code without side effects. My assumption is if Vite can bundle the code, it will do so. Why should there be a need to force it?

[benmccann]

@AlexRMU thanks, but unfortunately I can't make sense of what's going on from your comments here. You'll need to provide a reproduction in order for us to understand what possible issues might exist

@dominikg would it make sense to have vite-plugin-svelte warn a user if they put a Svelte library in external and the library is not prebundled?

[AlexRMU]

@benmccann
I used https://github.com/sveltejs/kit/pull/6301/files#diff-6366641a5812416d7ad8583e4e7f9bbb646f905873dcf2322678bdc84e220216R14 but I got errors (#6301 (comment) and Automattic/mongoose#12335)

[dominikg]

@dominikg would it make sense to have vite-plugin-svelte warn a user if they put a Svelte library in external and the library is not prebundled?

Can be done and makes sense given the footgun it has. Ideally we solve this problem by prebundling once that stabilized

@bluwy might have an opinion here too.

[bluwy]

Yeah I think it'd make sense to warn if the user is doing something unlikely to work. Prebundling in SSR may still have a long way to come.

[Rich-Harris]

After discussion, closing this in favour of #6372 which limits the scope of the changes to adapter-node