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

[rtsao]

Description

Fixes #9710

Any pnpm/Yarn workspace that depends on any CommonJS package from npm will crash SSR.

Minimal reproductions:

• https://github.com/rtsao/vite-bug-repro-ssr-pnpm (pnpm)
• https://github.com/rtsao/vite-bug-repro-ssr-pnp (Yarn)

The externalizer logic unconditionally attempts to resolve encountered dependencies from the project root. This works fine with hoisted node_modules installations, but does not work with more strict installation schemes where only explicit dependencies are only resolvable (such as pnpm or Yarn PnP). In this case, the externalizer is unable to resolve the sub-dependency from the root, and as a result it is not externalized for SSR.

For example, suppose we have the following dependency graph:
app -> dep-a -> dep-b

With npm, dep-b is resolvable from app (the root) because it is hoisted:

code
├── node_modules
│   ├── dep-a
│   └── dep-b
└── app

However, pnpm and PnP ensure strictness where dep-b cannot be resolved from app (and must be resolved from dep-a):

code
└── app
    └── node_modules
        └── dep-a
            └── node_modules
                └── dep-b

In this case, dep-b fails to be externalized for SSR (resulting in errors if dep-b is CJS)

This PR fixes the issue by passing along the importer and resolving from the importer, thereby allowing it to be resolved correctly.

Additional context

The externalization logic is memoized so resolution for a given package name only happens once. However, now resolution depends on two parameters: the package name and the importing path.

But taking into consideration the importing path would largely defeat the point of the cache as misses would be frequent.

I'm not super familiar with the intended logic of externalization, but I suppose a given package is OK to externalize as long as it was resolvable at some point. That being said, I think there probably needs to be more thinking about edge cases here, but I would need some help understanding the original context of this code.

---

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]

I think this is currently correctly enforced, that if a dep can be resolved from the project root (regardless of npm/pnpm), it will be externalized. Those that can't be resolved will be no-externalized. That also means there's a difference in the bundle output depending on npm/pnpm is used, but it shouldn't be an issue when executing the bundle code.

It seems like the core issue here is with linked deps being no externalized by default, since you've switch the file: protocol to workspace: (file: hardlinks), it reveals the error as Vite's SSR module loader doesn't handle module exports well, hence the ReferenceError: module is not defined error.

I think the correct solution here is to externalize the problematic CJS deps, which you've also found at #9710 (comment). Another easier way is to externalize the linked package, though note that you'll lost HMR when editing the package.

There has also been discussion to always externalize depending on linked or not, but that could be until Vite 4. I've added docs regarding this behaviour at https://main.vitejs.dev/guide/ssr.html#ssr-externals, though it's not updated in vitejs.dev yet.

[rtsao]

Hmm, it seems strange to me that it would be expected behavior to crash SSR in the common scenario of a pnpm or Yarn monorepo with CJS dependencies and default configuration (see https://github.com/rtsao/vite-bug-repro-ssr-pnpm and https://github.com/rtsao/vite-bug-repro-ssr-pnp).

IMHO there shouldn’t need to be configuration to prevent a crash in this case. CJS packages are the overwhelming majority of npm dependencies, especially in the realm of server-side dependencies. Manually adding and maintaining dozens (if not hundreds) of exceptions for every CJS dependency seems like an unreasonable burden.

[nitedani]

That also means there's a difference in the bundle output depending on npm/pnpm is used

I don't think that's ok.

[rtsao]

Are there any blockers to getting this merged that I can address? I'd be happy to work on whatever needs to be done.

This PR fixes a couple critical bugs with SSR (#9926, #9710) so I'd love to get this landed if possible.

[rtsao]

I've rebased the PR. I did have to add these vitepress devDependencies to the root to get Netlify docs build to work: vuejs/vitepress#849 (comment) because vitepress relies on node_modules hoisting to work correctly, which pnpm/Yarn don't allow. This previously worked because Vite was ignoring import importer context when resolving dependencies.

[patak-dev]

Adding this PR to the Vite 5 milestone so we merge or come to a conclusion of what should be done instead. Thanks for your patience here @rtsao

[bluwy]

Coming back to this, I think I misunderstood the initial problem before, and this fix sounds good to me. Thanks for setting up the repros to explain the problem.

Given this breaks the VitePress setup, I'm inclined to merge this in Vite 5 too.

[bluwy]

Actually looking deeper into this, I think there's still a way to make this non-breaking.

I've rebased the PR. I did have to add these vitepress devDependencies to the root to get Netlify docs build to work: vuejs/vitepress#849 (comment) because vitepress relies on node_modules hoisting to work correctly, which pnpm/Yarn don't allow. This previously worked because Vite was ignoring import importer context when resolving dependencies.

I don't think it's VitePress to blame here, we're applying a logic that would break all projects that noExternalizes a workspace dep (dep-a), and that workspace dep imports (dep-b) which this PR now externalizes.

This works great in dev for my app (app), but in build, what happens is that the final app bundle would bundle dep-a, but excludes dep-b. That final bundle would reference dep-b as-is in the context of app.

---

So what I think still needs to be fixed is that in build, we should still keep the old behaviour.

[rtsao]

Actually looking deeper into this, I think there's still a way to make this non-breaking.

I've rebased the PR. I did have to add these vitepress devDependencies to the root to get Netlify docs build to work: vuejs/vitepress#849 (comment) because vitepress relies on node_modules hoisting to work correctly, which pnpm/Yarn don't allow. This previously worked because Vite was ignoring import importer context when resolving dependencies.

I don't think it's VitePress to blame here, we're applying a logic that would break all projects that noExternalizes a workspace dep (dep-a), and that workspace dep imports (dep-b) which this PR now externalizes.

This works great in dev for my app (app), but in build, what happens is that the final app bundle would bundle dep-a, but excludes dep-b. That final bundle would reference dep-b as-is in the context of app.

So what I think still needs to be fixed is that in build, we should still keep the old behaviour.

Okay, I think I understand. I was confused because these errors happen during vitepress build, but it seems they are actually runtime ERR_MODULE_NOT_FOUND when executing bundled code.

In summary:

1. vitepress build uses vite build which bundles the app, and places the bundle within the host workspace (e.g. docs/.vitepress/.temp/app.js, as determined by https://github.com/vuejs/vitepress/blob/6acda7a6b343c872752fdae2af64da41768cf4ae/src/node/build/bundle.ts#L83)
2. Vite decides to externalize body-scroll-lock and @vueuse/core
3. When executing the bundle, runtime ERR_MODULE_NOT_FOUND occurs when attempting to resolve the externalized body-scroll-lock and @vueuse/core, because they are dependencies of vitepress and thus cannot be directly resolved from the docs workspace (in absence of hoisting).

I see few possible solutions:

• As you suggest, when bundling, do not externalize these dependencies. Can you help me better understand the externalization logic? What is the reason body-scroll-lock and @vueuse/core are externalized in the first place (at least in development)?

I think the appropriate modification to the externalization logic would be as follows:
If a resolved dependency has an import context that differs from the root app context, it should be disallowed from externalization during build (although not during unbundled dev).

Alternatively:

• Vitepress should instead place the bundled app within the vitepress workspace itself, so these externals can be resolved at runtime
• or, Vitepress should explicitly disallow externalization of these dependencies (because the bundle ends up the host app)

[bluwy]

• As you suggest, when bundling, do not externalize these dependencies. Can you help me better understand the externalization logic? What is the reason body-scroll-lock and @vueuse/core are externalized in the first place (at least in development)?

Before this PR, the externalization logic is that any dep that can be resolved from root gets external-ized, otherwise they get noExternalize-d. This is great for build as it's fixes the VitePress issue, and bundling workspace packages in general.

But it's not great for dev as you've shown in the issue. In dev, the ideal experience is that it checks from the importer, not the root, so that nested deps are correctly external/noExternal-ized.

The reason for this reverse behaviour is how Vite and Rollup externalization works in general. You can external-ized nested dep in dev because Vite's SSR runtime will preserve the importer context when resolving. You can't do so in build because Rollup's final bundle will not preserve the import context (bundled code is within root).

So the easiest fix is to only apply this fix in dev, and keep build as is. Or in other words, pass the importer only in dev, so that build will always resolve from root.

[brc-dd]

Just my two cents, it doesn't break just VitePress. It breaks every second project on the ecosystem CI. And I'm assuming the effect will be more considering on the ecosystem CI VP is passing (as VP has no tests where things are isolated). IMO this should not be merged without a major version bump.

[rtsao]

I've rebased and updated this fix to be a non-breaking change as discussed earlier (i.e. skip application of this change during build).

[patak-dev]

@bluwy should we move this PR from Vite 5 to 4.4 now?

[bluwy]

Yeah I think we can merge this in a minor now as it's non breaking.

[patak-dev]

/ecosystem-ci run

(expected to fail: iles, nuxt)

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
analogjs	✅ success
astro	✅ success
histoire	❌ failure
iles	❌ failure
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt	❌ failure
nx	✅ success
previewjs	✅ success
qwik	✅ success
rakkas	✅ success
sveltekit	❌ failure
unocss	✅ success
vite-plugin-pwa	✅ success
vite-plugin-ssr	✅ success
vite-plugin-react	✅ success
vite-plugin-react-pages	✅ success
vite-plugin-react-swc	✅ success
vite-plugin-svelte	✅ success
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	❌ failure
vitest	✅ success

[patak-dev]

It seems VitePress is still breaking with the PR 🤔

[brc-dd]

It seems VitePress is still breaking with the PR 🤔

It's probably not related to this, but instead to #13482 . I'll update the types there.

[brc-dd]

Should be fixed now.

[patak-dev]

/ecosystem-ci run vitepress

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
vitepress	✅ success

[heshamabdallah]

@rtsao It fails for nuxt, any update on this ?

We have 5+ projects ( we will have +10 by the end of the year ) and we built a monorepo to share logic between those projects and we have 12 packages in the monorepo and everyone of them has it's own dependencies. To fix this issue we currently install the external dependencies from the monorepo packages to the projects directly to get it work but it's very hard to keep track of dependencies versions and with every new installed dependency in the monorepo packages we have to install it to all the projects.. do you plan to work on the nuxt issue or do we raise an issue on their repo ? If you guide us how to fix it, that will be highly appreciated!