fix: bundle SvelteKit when using Vitest

[benmccann]

We had added SvelteKit to external in #5861 to make the tests pass as it was needed for Vite 3. It appears to no longer be necessary with Vite 4

This partially addresses #9162. There's still another issue I'll need to fix

[Rich-Harris]

actually i take back my approval — I don't understand why we're only adding @sveltejs/kit to noExternal if process.env.TEST is true, and I don't understand why esm-env would ever be external either. Feels like we could simplify stuff

[Rich-Harris]

#9203

[Rich-Harris]

The changeset needs changing but I'm not sure what to. What does this fix? Should it just be a chore:?

[benmccann]

What does this fix? Should it just be a chore:?

Well it currently doesn't fix anything because the actual fix got removed in #9203 😆 We need to add this section back:

			// Vitest will only call resolveId for packages that are being bundled
			// Without this it will not be able to load our virtual modules
			// See https://vitest.dev/config/#deps-registernodeloader
			const noExternal = process.env.TEST ? ['@sveltejs/kit'] : [];

I think we got our wires crossed on the discussion about that. I just tried explaining it another way and showed how you can verify that it helps here: #9162 (comment)

[Rich-Harris]

Have you confirmed that the change fixes the repro in #9162? Because I'm trying it locally and it doesn't appear to make any difference; nor would I expect it to per #9203 (comment)

[Rich-Harris]

Ah okay, it was making no difference because I was using a linked copy. Editing node_modules manually does change the behaviour — I'm surprised that Vite is interpreting noExternal: ['@sveltejs/kit'] as 'inline anything inside node_modules/@sveltejs/kit'

[Rich-Harris]

That being the case, we surely need @sveltejs/kit to always be noExternal. I'm at a loss as to why process.env.TEST is relevant

[benmccann]

Ah okay, it was making no difference because I was using a linked copy. Editing node_modules manually does change the behaviour

Vite automatically bundles any linked dependency, so yeah you wouldn't be able to see a difference in adding it to noExternal since it's already there when linked.

I'm surprised that Vite is interpreting noExternal: ['@sveltejs/kit'] as 'inline anything inside node_modules/@sveltejs/kit'

I don't really know what you mean by this. noExternal means don't externalize it, but bundle it, so isn't that exactly what the option does? Are you meaning something other than bundle when you say inline?

That being the case, we surely need @sveltejs/kit to always be noExternal

No, I don't think so. Why do you say that?

I'm at a loss as to why process.env.TEST is relevant

Because Vitest does not use Vite to load libraries that are not in noExternal. When using Vite it works just fine to have @sveltejs/kit in external. However, if you do that with Vitest then you're no longer using Vite which causes the breakage. We need @sveltejs/kit in noExternal if process.env.TEST in order to force Vitest to use Vite.

[Rich-Harris]

I don't really know what you mean by this

explained in #9203 (comment) — @sveltejs/kit refers specifically to this file which doesn't import __sveltekit/*. There are other files inside node_modules/@sveltejs/kit that do import those aliases, but those files are imported using absolute paths.

Why do you say that?

Either we can load this files through Node (in which case external is fine) or we can't (in which case they need to be noExternal). And in this case clearly we can't, because we need to resolve aliases

Because Vitest does not use Vite to load libraries that are not in noExternal

Isn't the whole point of noExternal that it causes libraries to be loaded through Vite, and if something isn't in noExternal (and doesn't satisfy whatever heuristic) it will be loaded through Node?

[benmccann]

Isn't the whole point of noExternal that it causes libraries to be loaded through Vite, and if something isn't in noExternal (and doesn't satisfy whatever heuristic) it will be loaded through Node?

external vs noExternal is about bundling vs not. I believe it doesn't affect whether something is loaded through Node and that Node is never used because I see that ids are always resolved. That may change in some future major version of Vite (There's vitejs/vite#12165 - though may not support Deno. There's also vitejs/vite#9740, but it sounds unlikely)

we surely need @sveltejs/kit to always be noExternal

Nope. Vite calls resolveId for all imports even if explicitly list @sveltejs/kit in external. Vitest on the otherhand uses Node for the external case, which is why this problem manifests only with Vitest.

Something else to note is that while @sveltejs/kit has:

	"peerDependencies": {
		"svelte": "^3.54.0",

vite-plugin-svelte is hardcoded to never add @sveltejs/kit to ssr.noExternal as an optimization (if it did then Vitest would work):
https://github.com/sveltejs/vite-plugin-svelte/blob/50db1401c7dc68ae9c06c7aa6edb68f09494bd2c/packages/vite-plugin-svelte/src/utils/dependencies.ts#L31

[Rich-Harris]

Vitest on the otherhand uses Node for the external case, which is why this problem manifests only with Vitest.

This sounds to me like a bug in Vitest. Why is it loading modules through a different mechanism than Vite itself?

[benmccann]

I'm not entirely sure. I believe it's for performance. The Vitest team sounded open to matching the behavior of Vite, but I don't think it will happen in the short-term. They're actively working to coordinate with Vite, trying to figure out where things should live, what high-level approaches to pursue, etc. That foundational stuff would probably need to be addressed first. E.g. see vitejs/vite#12165

In the meantime, it's probably worth adding a workaround for Vitest users given that we have so many of them and have been recommending Vitest in create-svelte.

[Rich-Harris]

ok, i added back the process.env.TEST check. i think this should be good to go now

[benmccann]

yeah, this looks good to me now. thanks!