force Vite to bundle component libraries in SSR #1148

Rich-Harris · 2021-04-20T12:43:49Z

fixes #904, by scanning all the package.json files of dependencies/devDependencies and adding any that have a "svelte" field to ssr.noExternal. The user's noExternal is still respected, so it's not a problem if this heuristic fails — it should just make these sorts of errors rarer.

This will fail in cases where a new component library is added a dependency while the dev server is running; in those cases turning it off and on again will fix it.

Finicky to add a test for this, but I verified it works with a site that uses external component libraries.

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

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

vercel · 2021-04-20T12:43:51Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sveltejs/kit-demo/3QjuiwcM3E3SaPxr96BcdmxoznMD
✅ Preview: https://kit-demo-git-gh-904-sveltejs1.vercel.app

benmccann · 2021-04-20T15:48:30Z

Should remove the last two sentences of https://github.com/sveltejs/kit/blob/master/documentation/faq/70-packages.md

benmccann · 2021-04-20T15:53:29Z

I actually wonder whether this is the right fix. I don't think Svelte files inherently need to be bundled. E.g. svelte-materialify works fine with Vite without bundling. It sounds to me like the issue with some Svelte components such as carbon-components-svelte is the same issue everyone else is hitting with regular libraries which is the Vite barfs upon encountering CommonJS vitejs/vite#2579

Rich-Harris · 2021-04-20T16:01:00Z

I get 'Must use import to load ES Module' when trying to develop kit.svelte.dev unless these lines are present: https://github.com/sveltejs/sites/blob/fd2c3c200065d07268e61ca113d793d115f82b78/sites/kit.svelte.dev/svelte.config.cjs#L12-L13

ssr: {
	noExternal:
		process.env.NODE_ENV === 'development'
			? ['@sveltejs/site-kit']
			: ['@sveltejs/site-kit', ...Object.keys(pkg.dependencies)]
}

With this fix that's no longer necessary. Not entirely sure how svelte-materialify is able to work, unless it actually is being bundled for some other reason

benmccann · 2021-04-20T16:08:48Z

Hmm. Interesting. Do you get a stack trace too or any other details?

Rich-Harris · 2021-04-20T18:29:16Z

Yes...

stack trace

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/richard/Development/SVELTE/sites/packages/site-kit/index.js
require() of ES modules is not supported.
require() of /Users/richard/Development/SVELTE/sites/packages/site-kit/index.js from /Users/richard/Development/SVELTE/sites/node_modules/.pnpm/vite@2.1.2/node_modules/vite/dist/node/chunks/dep-efe32886.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/richard/Development/SVELTE/sites/packages/site-kit/package.json.

    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1015:13)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at nodeRequire (/Users/richard/Development/SVELTE/sites/node_modules/.pnpm/vite@2.1.2/node_modules/vite/dist/node/chunks/dep-efe32886.js:68907:17)
    at ssrImport (/Users/richard/Development/SVELTE/sites/node_modules/.pnpm/vite@2.1.2/node_modules/vite/dist/node/chunks/dep-efe32886.js:68865:20)
    at eval (/Users/richard/Development/SVELTE/sites/sites/kit.svelte.dev/src/routes/$layout.svelte:9:31)
    at instantiateModule (/Users/richard/Development/SVELTE/sites/node_modules/.pnpm/vite@2.1.2/node_modules/vite/dist/node/chunks/dep-efe32886.js:68893:166)

...but it's not necessary, there's no mystery. require can't load components, which is why we have to add them to noExternal. this PR just does that for you

benmccann · 2021-04-20T19:28:08Z

It still seems like a mystery to me, but maybe I'm missing something. Why is it trying to require it instead of import it when site-kit has "type": "module"? And why does svelte-materialify not need to be bundled?

Rich-Harris · 2021-04-20T21:42:29Z

The heuristic is explained here: https://vitejs.dev/guide/ssr.html#ssr-externals. It's not using "type":"module". Even if it was, that wouldn't work because you can't import() a Svelte package any more than you can require() it. It has to be bundled, and that's what noExternal is for

Rich-Harris · 2021-04-20T21:42:46Z

As for svelte-materialify, I have no idea what it's doing

documentation/faq/70-packages.md

benmccann · 2021-04-21T05:40:42Z

documentation/faq/70-packages.md

 - [packages which use `exports` instead of `module.exports`](https://github.com/vitejs/vite/issues/2579).
 - [ESM library that imports a CJS library](https://github.com/vitejs/vite/issues/3024)
 - [some UMD libraries](https://github.com/vitejs/vite/issues/2679)

 Vite 2 is a relatively new library and over time we expect it to become easier to use non-ESM libraries with Vite. However, you might also consider asking the library author to distribute an ESM version of their package or even converting the source for the package entirely to ESM. ESM is now the standard way to write JavaScript libraries and while there are a lot of legacy packages out there the ecosystem will become much easier to work with as more libraries convert to ESM.

-The most common workarounds would be to try moving the package between `dependencies` and `devDependencies` or trying to `include` or `exclude` it in `optimizeDeps`. SvelteKit currently asks Vite to bundle all your `dependencies` for easier deployment to serverless environment. This means that by moving a dependency to `devDependencies` Vite is no longer asked to bundle it. This may sidestep issues Vite encounters in trying to bundle certain libraries. Avoiding Vite bundling especially works for `adapter-node` and `adapter-static` where the bundling isn't necessary since you're not running in a serverless environment. We are considering [better alternatives](https://github.com/sveltejs/kit/issues/1016) to make this setup easier. You should also add any Svelte components to `ssr.noExternal`. [We hope to do this automatically in the future](https://github.com/sveltejs/kit/issues/904).


Can we add back this description except for the part after "You should also add...". It's pretty confusing about what goes where and why and I think the extra explanation as to what's going on is helpful

It's out of date (though this did remind me to remove the vite.ssr block from svelte.config.cjs, so thanks). We don't ask Vite to bundle dependencies for serverless deployment; it bundles the stuff it thinks need to be bundled (based on its heuristic, the heuristic implemented in this PR, and the user's noExternal config), and adapters bundle everything else if they deem it necessary.

Technically speaking it was never correct; SvelteKit isn't asking for special treatment of dependencies, the project template was. Now that the vite.ssr config is removed from the template, swapping stuff between dependencies and devDependencies will make no difference, and nor should it — that was always a weird hack we embraced out of necessity.

Yeah, you're right that it was a bit imprecise about SvelteKit vs template

I didn't realize vite.ssr could be removed from the templates. I thought that was still needed as a trick @GrygrFlzr had introduced for bundling for deployment to serverless environments. I didn't think it could be removed without updating the adapters as described in #1016. If I'm wrong, maybe that issue can be closed now

Anyway, I'm happy it's gone. I just hope we didn't make it hard to use the adapters

pnpm-lock.yaml

benmccann · 2021-04-21T05:44:45Z

Ok, yeah, svelte-materialify is getting bundled because of the heuristics you mentioned. And Svelte components obviously need to be bundled. My head's been getting a bit jumbled trying to keep track of what Vite is doing in all these different scenarios

This looks good to me. I just left a few minor comments. Thanks for this!! Glad to have a bit of Vite complication handled automatically.

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>

force Vite to bundle component libraries in SSR - fixes #904

d6b6edc

get things working

6158086

vercel bot deployed to Preview April 20, 2021 15:46 View deployment

lockfile, again

dae6852

vercel bot deployed to Preview April 20, 2021 15:47 View deployment

update docs

ef60d0f

vercel bot deployed to Preview April 20, 2021 15:54 View deployment

benmccann requested a review from GrygrFlzr April 20, 2021 15:57

benmccann reviewed Apr 21, 2021

View reviewed changes

documentation/faq/70-packages.md Outdated Show resolved Hide resolved

benmccann reviewed Apr 21, 2021

View reviewed changes

pnpm-lock.yaml Show resolved Hide resolved

Update documentation/faq/70-packages.md

3c5083b

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>

vercel bot deployed to Preview April 22, 2021 01:27 View deployment

Rich-Harris added 3 commits April 21, 2021 21:28

remove ssr config from svelte.config.cjs

5bef860

Merge branch 'gh-904' of github.com:sveltejs/kit into gh-904

e1049ed

changeset

5cecd8a

vercel bot deployed to Preview April 22, 2021 01:36 View deployment

Rich-Harris merged commit 4645ad5 into master Apr 22, 2021

Rich-Harris deleted the gh-904 branch April 22, 2021 01:47

benmccann mentioned this pull request Apr 22, 2021

Repeated errors from [vite] after pnpm init svelte@next my-app (2.0.0-next.55): Error when evaluating SSR module cookie: ReferenceError: exports is not defined #1113

Closed

benmccann mentioned this pull request Apr 22, 2021

Don't bundle dependencies during build and remove noExternal default #1016

Closed

This was referenced Jun 10, 2021

Add faq docs about limitation in vite-plugin-svelte #1665

Merged

Svelte-Kit: Can't use Image component matyunya/svelte-image#109

Open

ignatiusmb mentioned this pull request Jul 9, 2021

default export from svelte-kit package #1856

Closed

Rich-Harris mentioned this pull request Nov 23, 2021

Remove the need for an index file with npm run package #2884

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

force Vite to bundle component libraries in SSR #1148

force Vite to bundle component libraries in SSR #1148

Rich-Harris commented Apr 20, 2021

vercel bot commented Apr 20, 2021 •

edited

Loading

benmccann commented Apr 20, 2021

benmccann commented Apr 20, 2021

Rich-Harris commented Apr 20, 2021

benmccann commented Apr 20, 2021 •

edited

Loading

Rich-Harris commented Apr 20, 2021

benmccann commented Apr 20, 2021

Rich-Harris commented Apr 20, 2021

Rich-Harris commented Apr 20, 2021

benmccann Apr 21, 2021

Rich-Harris Apr 22, 2021

benmccann Apr 22, 2021

benmccann commented Apr 21, 2021

force Vite to bundle component libraries in SSR #1148

force Vite to bundle component libraries in SSR #1148

Conversation

Rich-Harris commented Apr 20, 2021

Before submitting the PR, please make sure you do the following

Tests

Changesets

vercel bot commented Apr 20, 2021 • edited Loading

benmccann commented Apr 20, 2021

benmccann commented Apr 20, 2021

Rich-Harris commented Apr 20, 2021

benmccann commented Apr 20, 2021 • edited Loading

Rich-Harris commented Apr 20, 2021

benmccann commented Apr 20, 2021

Rich-Harris commented Apr 20, 2021

Rich-Harris commented Apr 20, 2021

benmccann Apr 21, 2021

Choose a reason for hiding this comment

Rich-Harris Apr 22, 2021

Choose a reason for hiding this comment

benmccann Apr 22, 2021

Choose a reason for hiding this comment

benmccann commented Apr 21, 2021

vercel bot commented Apr 20, 2021 •

edited

Loading

benmccann commented Apr 20, 2021 •

edited

Loading