Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jun 13, 2023

Conversation

rtsao
Copy link
Contributor

@rtsao rtsao commented Aug 19, 2022

Description

Fixes #9710

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

Minimal reproductions:

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.

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr labels Aug 20, 2022
@bluwy
Copy link
Member

bluwy commented Aug 21, 2022

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
Copy link
Contributor Author

rtsao commented Aug 22, 2022

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
Copy link

nitedani commented Sep 20, 2022

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
Copy link
Contributor Author

rtsao commented Oct 19, 2022

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.

@benmccann benmccann changed the title fix(ssr): ensure sup-dependencies are correctly externalized if not resolvable from root fix(ssr): ensure sub-dependencies are correctly externalized if not resolvable from root Oct 28, 2022
@rtsao rtsao force-pushed the fix-externals-pnp branch 3 times, most recently from 2867a32 to 1b94406 Compare April 3, 2023 17:11
@rtsao
Copy link
Contributor Author

rtsao commented Apr 3, 2023

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.

@rtsao rtsao changed the title fix(ssr): ensure sub-dependencies are correctly externalized if not resolvable from root fix(ssr): fix crash when a pnpm/Yarn workspace depends on a CJS package Apr 4, 2023
@rtsao rtsao force-pushed the fix-externals-pnp branch 3 times, most recently from a12c08d to ef93e1c Compare April 6, 2023 19:34
@patak-dev patak-dev added this to the 5.0 milestone Apr 6, 2023
@patak-dev
Copy link
Member

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
Copy link
Member

bluwy commented Apr 8, 2023

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
Copy link
Member

bluwy commented Apr 8, 2023

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
Copy link
Contributor Author

rtsao commented Apr 10, 2023

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
Copy link
Member

bluwy commented Apr 13, 2023

  • 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
Copy link
Contributor

brc-dd commented Apr 22, 2023

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 rtsao marked this pull request as draft June 1, 2023 18:00
@rtsao rtsao marked this pull request as ready for review June 12, 2023 22:45
@rtsao
Copy link
Contributor Author

rtsao commented Jun 12, 2023

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
Copy link
Member

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

@bluwy
Copy link
Member

bluwy commented Jun 13, 2023

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

@patak-dev patak-dev modified the milestones: 5.0, 4.4 Jun 13, 2023
@patak-dev
Copy link
Member

/ecosystem-ci run

(expected to fail: iles, nuxt)

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jun 13, 2023

📝 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
Copy link
Member

It seems VitePress is still breaking with the PR 🤔

@brc-dd
Copy link
Contributor

brc-dd commented Jun 13, 2023

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
Copy link
Contributor

brc-dd commented Jun 13, 2023

Should be fixed now.

@patak-dev
Copy link
Member

/ecosystem-ci run vitepress

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jun 13, 2023

📝 Ran ecosystem CI: Open

suite result
vitepress ✅ success

@patak-dev patak-dev merged commit 9e1086b into vitejs:main Jun 13, 2023
@heshamabdallah
Copy link

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CommonJS dependencies crash SSR server
7 participants