fix: ssrExternal should not skip nested dependencies

[benmccann]

Description

ssrExternal is skipping nested dependencies causing breakages during SSR

Additional context

Before digging into this, I suspected it was an issue in vite-plugin-svelte and filed an issue there: sveltejs/vite-plugin-svelte#281. It turned out to be an issue in Vite instead

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

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

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[bluwy]

This LGTM. I don't see any edge case with this at the moment, but would be a good candidate for the beta.

[bluwy]

I just realized something. When vite-plugin-svelte starts providing support to prebundle svelte libraries, this wouldn't really be useful anymore since we don't add optimizeDeps.include with the > syntax. That means we'd still need sveltejs/vite-plugin-svelte#289, but I wonder if we need this PR now. This PR would be useful if the parent is added to ssr.noExternal, but since the > comes from optimizeDeps.include, they aren't quite related.

[benmccann]

Even if Svelte doesn't use the > syntax, it's still a documented feature of Vite and the bug would still exist, so I think it's probably a good change to make either way

[bluwy]

I don't think there's a bug since Vite currently ignores these sort of syntax as expected. We can still merge this though, but it leads to extra valid ssr.external specifiers in the logic, which could be fine in most normal usecase, but we should also have vite-plugin-svelte not rely on this behaviour (and other plugins if there is)

[benmccann]

Is it expected that Vite ignores dependencies with the > syntax? I don't think it's expected, which is why I said there's a bug. I don't think they're "extra", but rather are expected to be there, but maybe I'm mistaken?

[bluwy]

Yeah I think it is expected that Vite ignores > right now. The reason Vite uses metadata.optimized now is to retrieved a list of known imports or dependencies after scanning. So it only cares about imports without the > syntax, as in JS there's no imports with the > syntax. The > syntax is only used in user configs or plugins, and inadvertently enters the SSR externalization logic.

But putting it another way, if we have optimizeDeps.include: ["foo > bar"] and ssr.noExternal: ["foo"], where foo is an ESM dep, and bar a CJS dep. I shouldn't expect removing optimizeDeps.include to have an impact on ssrLoadModule. Right now, if regardless of optimizeDeps.include it would always error out as expected. But with this PR, keeping the optimizeDeps.include lets ssrLoadModule work, while removing it would cause an error, which I feel is a bit inconsistent.

[benmccann]

Hmm. The plugins are adding the > dependencies because import scanning can't find them due to unsupported file types. I suppose that might be addressed by running the Svelte plugin inside esbuild.

, if we have optimizeDeps.include: ["foo > bar"] and ssr.noExternal: ["foo"], where foo is an ESM dep, and bar a CJS dep. I shouldn't expect removing optimizeDeps.include to have an impact on ssrLoadModule. Right now, if regardless of optimizeDeps.include it would always error out as expected. But with this PR, keeping the optimizeDeps.include lets ssrLoadModule work, while removing it would cause an error, which I feel is a bit inconsistent.

I'm not sure I would expect this to error out regardless of what optimizeDeps contains. I would expect the import scanning to find both packages and externalize them both. Is that wrong?

[bluwy]

Hmm. The plugins are adding the > dependencies because import scanning can't find them due to unsupported file types. I suppose that might be addressed by running the Svelte plugin inside esbuild.

We're currently adding > as we have to optimizeDeps.exclude Svelte libraries, but re-optimizeDeps.include the non-ESM deps of the Svelte library. Vite is able to scan Svelte files but it doesn't scan into libraries in node_modules.

I'm not sure I would expect this to error out regardless of what optimizeDeps contains. I would expect the import scanning to find both packages and externalize them both. Is that wrong?

The import scanning currently only does a shallow scan (it doesn't scan the bar transitive dependency), so it will only find foo. In most cases this is fine, as foo will be loaded by Node. But if we do ssr.noExternal: ['foo'], foo will not be loaded by Node, but goes through Vite's plugin system to "process" the package, and this is needed because Node doesn't understand how to import Svelte files.

However, in doing so the dependencies of foo, e.g. bar will be imported. Vite handles this part interestingly. Because bar is not in ssr.external, Vite will modify the import as import bla from '/path/.../bar/index.js'. In cases like this, Node will import it directly without making sure it's ESM. Only when the dep is in ssr.external, the import will be kept as import bla from 'bar', where Vite will ensure it's ESM. (Code)

This is my observation so far though. Maybe Vite should be auto-externalizing transitive deps by default, but I'm not sure how to fix this yet.

[benmccann]

Only when the dep is in ssr.external, the import will be kept as import bla from 'bar', where Vite will ensure it's ESM.

Vite can and should import both CJS and ESM code. You linked to a line of code calling nodeImport in your last comment. If you read the jsdoc for that method it says it that it works with CJS as well.

Maybe Vite should be auto-externalizing transitive deps by default, but I'm not sure how to fix this yet.

Yes, I think Vite should externalize everything by default. I took a stab at that earlier (#6698), but one of the tests was failing and I didn't have time to investigate.

[bluwy]

I've been thinking about this again, and I suppose this should be fine to merge. The reason I was initially oppose was that having parent > child's child part externalized would externalize it everywhere in the code, but I think getting more known imports shouldn't cause any side effects since the more we can use Node to import, the better.