Configure transitive dependencies in ssr external/noExternal

[benmccann]

Description

SvelteKit needs to add Svelte libraries to noExternal to compile the .svelte files. We'd also like devDependencies to be bundled and dependencies not to be. This is because when people run npm install in production they're not getting their devDependencies installed, so it'd be nice to bundle those.

This is almost impossible because we can't just add the devDependencies to noExternal since it doesn't also add the transitive dependencies.

We can't set noExternal to true and add the dependencies to external because external takes precedence making it easy for a user to add a Svelte library to dependencies/external which would cause it to break since Svelte needs to be bundled (proposed in sveltejs/kit#6301). If you do the reverse and add devDependencies to noExternal it still allows vite-plugin-svelte to add things to noExternal without harsh overrides

It also turns out that noExternal: true is basically impossible to use because it validates that you don't import any Node built-ins, which is maybe useful for deploying to a target like Cloudflare workers, but quite often is a frustrating restriction. (Edit: turns out you can work around that sveltejs/kit#6301 (comment))

Suggested solution

The ssr.external and ssr.noExternal docs are fairly vague. If my recollection serves me, one deals with libraries and one with .js files. It would be nice to clarify what they operate on

Given my current lack of clarity as to how these options work, it's hard for me to suggest solutions. Is it possible for the options to take effect not just for the individual libraries, but also their dependencies?

Alternative

This is currently being worked around by bundling the Vite output with esbuild, but that seems like it should be unnecessary. The second bundling step adds an additional layer of complexity and causes users to hit all of esbuild's bugs (e.g. sveltejs/kit#6440)

Additional context

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

[patak-dev]

What we have right now (both in Vite 2 and Vite 3) is:

• ssr.noExternal expects packages names.
• ssr.external works both with packages names and with direct entries.

Dependencies of the project are external by default, so if we have:

• project
  -> dep_A -> (depends on dep_B)
  -> dep_B

And dep_A is configured as noExternal, dep_B will still be external because it is a dep of the project. But if we have

• project
  -> dep_A -> (depends on dep_B)

Then both dep_A and dep_B will be noExternal. So if the user wants both dep_A and dep_B in the first case to be noExternal, both will need to be added to ssr.noExternal

If I recall correctly, at one point Vite did differentiate between dependencies and devDependencies. Maybe it was wrong to treat them in the same way, and devDependencies should be noExternal by default that is what you would like to achieve here. Let's discuss this with others, a change here could only be applied in Vite 4 I think though

[benmccann]

Yeah, I sent the PR to switch it about a year ago: #4699. It looks like part of the issue back then was that we were always using require which would fail with ESM libraries. However, now we always use import, so that should no longer be a concern. However, the other part of the issue is that dev mode bundling often fails and would make the development experience slower. As a result, we're currently hoping to only turn it on for build.

Your comment suggested that noExternal is already applied transitively and the way you described it would be perfect for us. However, one of the Svelte maintainers reported otherwise in sveltejs/kit#3176 (comment) which is why we added the second bundling step with esbuild. Do you know where in the code the transitive dependencies would be included? I could try to debug it and see why they weren't in our case.

[patak-dev]

@benmccann I think this PR, that is trying to go in the opposite way of what you want here, is a good place to see how this works:

• fix(ssr): fix crash when a pnpm/Yarn workspace depends on a CJS package #9763

Here is the code that is trying to resolve the dep from root, and filters subdependencies as noExternal:

vite/packages/vite/src/node/ssr/ssrExternal.ts

Line 133 in 7c4ab51

return !!tryNodeResolve(

cc @bluwy, in Vite 4, shouldn't we evaluate having devDependencies be noExternal by default? This threads into the discussion we had with @brillout about SSR by default bundling everything that may not be installed in prod later (or that depends on the used package manager)

[benmccann]

In the case I'm looking at @sveltejs/kit depends on devalue. When shouldExternalizeForSSR checks devalue it returns true via the code path below.

shouldExternalizeForSSR:

vite/packages/vite/src/node/ssr/ssrExternal.ts

Line 106 in 561f7de

return isSsrExternal(id, importer)

Calls function returned by createIsSsrExternal:

vite/packages/vite/src/node/ssr/ssrExternal.ts

Line 205 in 561f7de

external = isBuiltin(id) || isConfiguredAsExternal(id, importer)

Calls function returned by createIsConfiguredAsSsrExternal:

vite/packages/vite/src/node/ssr/ssrExternal.ts

Line 188 in 561f7de

return isExternalizable(id, importer)

Calls isExternalizable which returns true:

vite/packages/vite/src/node/ssr/ssrExternal.ts

Line 135 in 561f7de

return !!tryNodeResolve(

I might be missing it, but I don't see anywhere in here where noExternal would be applied transitively. If we could change that so that it were applied transitively that would address the issue we're facing.

[bluwy]

cc @bluwy, in Vite 4, shouldn't we evaluate having devDependencies be noExternal by default? This threads into the discussion we had with @brillout about SSR by default bundling everything that may not be installed in prod later (or that depends on the used package manager)

I think the idea so far is to not give special treatment to dependencies and devDependencies, which is good to prevent surprises when adding dependencies. Similar to the discussion where we don't want linked deps to be auto noExternal by default, I think we should also avoid prod and dev deps having special meaning too (?) 🤔

I might be missing it, but I don't see anywhere in here where noExternal would be applied transitively. If we could change that so that it were applied transitively that would address the issue we're facing.

Yeah noExternal currently isn't applied transitively because it doesn't quite work (or maybe I misunderstand the scenario). For example, foo depends on bar, if foo is externalized, it will be handled by node entirely down the tree which we can't rewrite it's bar dependency to the bundled version.

[benmccann]

Yeah noExternal currently isn't applied transitively because it doesn't quite work (or maybe I misunderstand the scenario). For example, foo depends on bar, if foo is externalized, it will be handled by node entirely down the tree which we can't rewrite it's bar dependency to the bundled version.

I think it would still be possible given the example you shared. If the user explicitly externalizes foo then it's fine for that to take precedence transitively and for both foo and bar to be externalized.

Another thing I'll mention is that if we have foo depends on bar depends on baz, the only thing I really care about configuring is foo. I've never really had a need to configure bar or baz.

Handling things transitively would also add some simplifications. E.g. we wouldn't need the > syntax to be able to manually specify every transitive dependency in vite-plugin-svelte because it would happen automatically.