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

CommonJS dependencies crash SSR server #9710

Closed
7 tasks done
rtsao opened this issue Aug 16, 2022 · 13 comments · Fixed by #9763
Closed
7 tasks done

CommonJS dependencies crash SSR server #9710

rtsao opened this issue Aug 16, 2022 · 13 comments · Fixed by #9763
Assignees
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@rtsao
Copy link
Contributor

rtsao commented Aug 16, 2022

Describe the bug

CommonJS dependencies encountered during SSR can crash the server (e.g. ReferenceError: module is not defined or ReferenceError: require is not defined, depending on which is first) if workspaces are used in some capacity.

I have identified two specific scenarios where this will occur:

  1. The CJS dependency is a workspace (i.e. appcjs-workspace)
  2. The CJS dependency is a sub-dependency of a pnpm or Yarn PnP workspace dependency (i.e. appother-workspacecjs-npm-dependency)

While possibly confusing, the first scenario is arguably intentional behavior because workspaces are typically implemented with symlinks and this logic indicates that symlinked packages should never be externalized:

// don't external symlink packages
if (!allowLinkedExternal && !resolved.id.includes('node_modules')) {
return resolved
}

However, the second scenario is almost certainly a bug.

Reproduction

https://github.com/rtsao/vite-bug-repro-cjs-workspaces (for scenario 1)
https://github.com/rtsao/vite-bug-repro-ssr-pnp and https://github.com/rtsao/vite-bug-repro-ssr-pnpm (for scenario 2)

System Info

System:
    OS: macOS 12.5
    CPU: (10) arm64 Apple M1 Max
    Memory: 36.02 GB / 64.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.16.0 - ~/Library/Caches/fnm_multishells/1157_1660588473591/bin/node
    Yarn: 3.2.2 - ~/Library/Caches/fnm_multishells/1157_1660588473591/bin/yarn
    npm: 8.11.0 - ~/Library/Caches/fnm_multishells/1157_1660588473591/bin/npm
  Browsers:
    Chrome: 104.0.5112.101
    Safari: 15.6

Used Package Manager

yarn

Logs

Click to expand!
vite-bug-repro-ssr-pnp main
❯ corepack enable
yarn install
yarn node app/server.mjs
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0013: │ vite@npm:3.0.8 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ which@npm:2.0.2 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ wide-align@npm:1.1.5 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ wrappy@npm:1.0.2 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ yallist@npm:4.0.0 can't be found in the cache and will be fetched from the remote registry
➤ YN0000: └ Completed
➤ YN0000: ┌ Link step
➤ YN0000: │ ESM support for PnP uses the experimental loader API and is therefore experimental
➤ YN0007: │ esbuild@npm:0.14.54 must be built because it never has been before or the last one failed
➤ YN0000: └ Completed in 0s 616ms
➤ YN0000: Done with warnings in 0s 788ms
http://localhost:5173
ReferenceError: module is not defined
    at /@fs/Users/rtsao/code/vite-bug-repro-ssr-pnp/.yarn/cache/locale-npm-0.1.0-6c7fdc5891-42af4432bc.zip/node_modules/locale/lib/index.js:159:3
    at /@fs/Users/rtsao/code/vite-bug-repro-ssr-pnp/.yarn/cache/locale-npm-0.1.0-6c7fdc5891-42af4432bc.zip/node_modules/locale/lib/index.js:161:4
    at instantiateModule (file:///Users/rtsao/code/vite-bug-repro-ssr-pnp/.yarn/__virtual__/vite-virtual-f00fbd239e/0/cache/vite-npm-3.0.8-cf480b5a77-ec3f57d52f.zip/node_modules/vite/dist/node/chunks/dep-74663fff.js:50512:15)
ReferenceError: module is not defined
    at /@fs/Users/rtsao/code/vite-bug-repro-ssr-pnp/.yarn/cache/locale-npm-0.1.0-6c7fdc5891-42af4432bc.zip/node_modules/locale/lib/index.js:157:3
    at /@fs/Users/rtsao/code/vite-bug-repro-ssr-pnp/.yarn/cache/locale-npm-0.1.0-6c7fdc5891-42af4432bc.zip/node_modules/locale/lib/index.js:159:4
    at instantiateModule (file:///Users/rtsao/code/vite-bug-repro-ssr-pnp/.yarn/__virtual__/vite-virtual-f00fbd239e/0/cache/vite-npm-3.0.8-cf480b5a77-ec3f57d52f.zip/node_modules/vite/dist/node/chunks/dep-74663fff.js:50512:15)

Validations

@benmccann benmccann added bug p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr and removed pending triage labels Aug 16, 2022
@rtsao rtsao changed the title CommonJS sub-dependencies crash SSR server CommonJS dependencies crash SSR server Aug 17, 2022
@sun0day
Copy link
Member

sun0day commented Aug 17, 2022

It seems like Vite has not supported ssr pre bundle yet. See https://github.com/vitejs/vite/blob/main/packages/vite/src/node/optimizer/optimizer.ts#L724

@patak-dev
Copy link
Member

There is now support for prebundling SSR deps during dev, which should give a way out of these issues. You need to manually enable it, and add these dependencies to the include list:

ssr: {
  optimizeDeps: {
    disabled: 'build'
    include: [ 'dep' ]
  }
}

@sun0day
Copy link
Member

sun0day commented Aug 17, 2022

It didn't work, did I miss something? @patak-dev

There is now support for prebundling SSR deps during dev, which should give a way out of these issues. You need to manually enable it, and add these dependencies to the include list:

ssr: {
  optimizeDeps: {
    disabled: 'build'
    include: [ 'dep' ]
  }
}

@patak-dev
Copy link
Member

Just in case @sun0day, 'dep' was a placeholder, did you replace that with the dependency you want to optimize? Please share a minimal reproduction of your setup if that is the case

@sun0day
Copy link
Member

sun0day commented Aug 17, 2022

Just in case @sun0day, 'dep' was a placeholder, did you replace that with the dependency you want to optimize? Please share a minimal reproduction of your setup if that is the case

I just clone https://github.com/rtsao/vite-bug-repro-ssr-pnp mentioned above, and add

ssr: {
    optimizeDeps: {
      disabled: 'build',
      include: [ 'pkg-a' ]
    }
  }

to app/vite.config.js. Then follow the reproduction steps as README.md says.

@rtsao
Copy link
Contributor Author

rtsao commented Aug 17, 2022

I'm aware of a workaround to forcibly externalize these deps (as they should be): {ssr: {external: ["the-dep"]}}) but this is rather cumbersome for large projects where there could be hundreds of CommonJS sub-dependencies. Very few npm packages are ESM.

I think the heuristics for whether dependencies should be externalized is broken somehow.

There is now support for prebundling SSR deps during dev

Why bundle server dependencies? Isn't this just overhead?

@rtsao
Copy link
Contributor Author

rtsao commented Aug 19, 2022

Put together a fix: #9763

TL;DR: the current externalization method of attempting to resolve all nested deps from root is broken with pnpm and Yarn PnP

@arthurgailes
Copy link

@rtsao Thank you. This also appears to affect nx workspaces, in my case with the package d3.

@arthurgailes
Copy link

arthurgailes commented Aug 24, 2022

I'm aware of a workaround to forcibly externalize these deps (as they should be): {ssr: {external: ["the-dep"]}}) but this is rather cumbersome for large projects where there could be hundreds of CommonJS sub-dependencies. Very few npm packages are ESM.

I think the heuristics for whether dependencies should be externalized is broken somehow.

There is now support for prebundling SSR deps during dev

Why bundle server dependencies? Isn't this just overhead?

Thank you, this actually worked for me as {ssr: {noExternal: ["the-dep"]}}

Overall, it took several hours to track down the cause here - thank you for your PR on the subject.

@edikdeisling
Copy link
Contributor

edikdeisling commented Feb 28, 2023

Run into this issue. I have case 2 with pnpm: workspace-package-1 -> workspace-package-2 -> jsdom.
Adding {ssr: {noExternal: ["jsdom"]}} or {ssr: {optimizeDeps: {disabled: 'build', include: [ 'jsdom' ]}}} doesn't help.

But for some reason adding jsdom as a dependency for workspace-package-1 helped. So for now I added all these broken dependencies to both workspace packages

@rtsao
Copy link
Contributor Author

rtsao commented Mar 29, 2023

@volta-net volta-net bot assigned rtsao Apr 4, 2023
@rtsao
Copy link
Contributor Author

rtsao commented Apr 4, 2023

I've rebased the fix for this issue: #9763 and is passing CI again

@vincenteof
Copy link

There is now support for prebundling SSR deps during dev, which should give a way out of these issues. You need to manually enable it, and add these dependencies to the include list:

ssr: {
  optimizeDeps: {
    disabled: 'build'
    include: [ 'dep' ]
  }
}

I add this config for @prisma/client and the resolution works fine, but seems that prisma is using __dirname in its source, and error says __dirname is not defined, what can I do?

@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants