fix: respect explicitily external/noExternal config

[patak-dev]

Description

Fixes #8974 when vite-plugin-ssr is explicitely configured as ssr.external

@brillout following the previous logic, it looks like defining a dependency as noExternal wasn't affecting its internal dependencies. Is this what is expected?

Imagine if vite-plugin-ssr is ESM, when you mark vikepress as noExternal, do you expect vite-plugin-ssr to also be noExternal?

This is how optimizeDeps.exclude works, the whole dep tree is excluded according to @bluwy. That is why we need a optimizeDeps.include: 'entry > sub' if we want to escape that.

We could make noExternal work like optimizeDeps.exclude, but I think at least for v3, just making everything external by default is ok. This means that you shouldn't need to explicitly configure vite-plugin-ssr as external, as noExternal only affects a single node imported file and not the imported modules from it. Let me know if this sounds ok to you too.

The current code, once this PR is merged should already work like this. But there is another bug at play that prevents vite-plugin-ssr to be detected as external. Looks like this tryNodeResolve call:

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

Line 153 in 5c6cf5a

return !!tryNodeResolve(

is failing. @bluwy saw that it is working if vite-plugin-ssr is also installed as an entry.

I'm working on a refactoring, I think we should directly check do the external/noExternal logic inside tryNodeResolve. Pushing this PR to keep the conversation going in the mean time.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[netlify]

✅ Deploy Preview for vite-docs-main canceled.

Name	Link
🔨 Latest commit	1699ab8
🔍 Latest deploy log	https://app.netlify.com/sites/vite-docs-main/deploys/62c7e81ac262430008690b5a

[bluwy]

We could make noExternal work like optimizeDeps.exclude, but I think at least for v3, just making everything external by default is ok. This means that you shouldn't need to explicitly configure vite-plugin-ssr as external, as noExternal only affects a single node imported file and not the imported modules from it. Let me know if this sounds ok to you too.

I think we should make noExternal work like optimizeDeps.exclude now though, so if we noExternal vikepress, vite-plugin-ssr would be noExternal too. Because after building, the output code can't import vite-plugin-ssr as it's not a direct dependency.

[patak-dev]

I think we should make noExternal work like optimizeDeps.exclude now though, so if we noExternal vikepress, vite-plugin-ssr would be noExternal too. Because after building, the output code can't import vite-plugin-ssr as it's not a direct dependency.

So vite-plugin-ssr needs to be a peer dep of vikepress to be able to externalize it. I think it isn't bad that it is external by default even if vikepress is marked as no external in this case, as Vite should prefer external as possible. If we have an installed dep, better it be external unless explicitly marked as noExternal

[brillout]

On a high-level I think we should only noExternal VikePress while vite-plugin-ssr is externalized. Because we want Vite to externalize as many SSR dependencies as possible. That's what I've have been pushing for with e.g. "always externalize SSR deps by default". CC @benmccann which as been as well pushing for such default.

Now @bluwy has a point that if VikePress is noExternal then VikePress's code lives in node_modules/.vite/[edit] dist/ and therefore cannot import its direct dependencies (e.g. import 'vite-plugin-ssr') when using pnpm without shamefully-hoist=true.

One way we can solve this is to resolve the import statements in node_modules/.vite/[edit] dist/: instead of having import 'vite-plugin-ssr' we would have import '../../node_modules/.pnpm/[email protected]_biqbaboplfbrettd7655fr4n2y/node_modules/vite-plugin-ssr'.

CC @benmccann SvelteKit also have the situation that some SSR dependencies need to be noExternal. What do you think?

[patak-dev]

One way we can solve this is to resolve the import statements in node_modules/.vite/: instead of having import 'vite-plugin-ssr' we would have import '../.pnpm/[email protected]_biqbaboplfbrettd7655fr4n2y/node_modules/vite-plugin-ssr'.

Why don't make 'vite-plugin-ssr' a peer dependency? Seems like something users should install in their projects. VikePress code doesn't live in node_modules/.vite/ as it won't be optimized because you want to use Vite features on it. VikePress is noExternal and ssr.exclude so the code will live in your dist folder.

[brillout]

Ah, yes, sorry it lives in dist/ not .node_modules/.vite/. The problem is the same though.

For the vite-plugin-ssr dependency, yes, having it as a peer dependency would be an option. But that's not the case for other dependencies: for example the twemoji dep should not be a peer dep.

[patak-dev]

For the vite-plugin-ssr dependency, yes, having it as a peer dependency would be an option. But that's not the case for other dependencies: for example the twemoji dep should not be a peer dep.

Why twemoji works as external in v2 if it isn't installed? v2 will externalize it, but then it could fail depending on the used pm, no? v3 is now more strict here, you could place twemoji in external and after this PR it will be externalized even if it isn't installed and you'll need to use a setup that supports that. Changing the paths would count as a feature request, no?
Are we ok with the more strict check we have now in v3? v3 is making more entries external by default, but internal deps are now noExternal if not installed. This is a departure from how things worked in v2

[bluwy]

I'm not really sure about rewriting imports for externalized deps since it makes the output code not portable, and agree with patak that we could discuss this separately since this feature doesn't exist in Vite 2 too.

[patak-dev]

Another note, in v2, twemoji would have worked in dev SSR depending on your setup. In v3 it is not going to externalize it if it isn't installed also for dev SSR.

[brillout]

I just checked and v2 actually does noExternalize twemoji. So it works regardless of the pm.

In v3 it is not going to externalize it if it isn't installed

👍 Seems like a reasonable default.

Are we ok with the more strict check we have now in v3?

From my side, yes. (VikePress is a low-key project anyways.)

But I think we should check with the SvelteKit folks. (Svelte libraries can contain .svelte source files; they must use ssr.noExternal as well.)

we could discuss this separately since this feature doesn't exist in Vite 2 too

👍 #8991.

[patak-dev]

@brillout running brillout/vite-3-legacy-build-bug against this PR properly externalizes vite-plugin-ssr, and then we get:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'vite-plugin-ssr' imported from /Users/patak/test/vite-3-legacy-build-bug/dist/server/assets/chunk-93b94dab.js

Is my understanding correct that after this PR is merged and you update vikepress, CI should be green for vite-plugin-ssr?

Let's merge this PR as we should respect users explicit external/noExternal configs independently of any other changes we will propose later.

About this, we may still have a bug lurking, but we'll need a different issue and reproduction if this is still a problem:

Note that the reproduction is green in vite-plugin-ssr's CI (the reproduction is a subset of the CI). The difference with the CI is that vite-plugin-ssr is symlinked in the CI. So I'm suspecting some kind of node_modules if-branch to be the culprit.

[brillout]

Yes. I'll update the vite-plugin-ssr CI bug reports as soon as there is a new beta release.