Dual package hazard in cjs monorepo

[sukovanej]

Describe the bug

Related issue: Effect-TS/effect#1548

In the setup (check the repro), vitest loads both the esm and cjs versions of the effect package.

cc @Andarist

Reproduction

https://github.com/sukovanej/effect-cjs-yarn-monorepo-repro

System Info

System:
    OS: macOS 14.0
    CPU: (8) arm64 Apple M2
    Memory: 79.05 MB / 16.00 GB
    Shell: 3.6.1 - /opt/homebrew/bin/fish
  Binaries:
    Node: 20.8.1 - /opt/homebrew/bin/node
    Yarn: 4.0.0 - /usr/local/bin/yarn
    npm: 10.1.0 - /opt/homebrew/bin/npm
  Browsers:
    Chrome: 118.0.5993.88
    Safari: 17.0
  npmPackages:
    vitest: ^0.34.6 => 0.34.6

Used Package Manager

yarn

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[sheremet-va]

Vitest separates files into two categories - source files and external files.

External files are loaded with the native Node.js mechanism without interceptions (simple dynamic import) for better performance.

Source files are loaded using vite-node which uses Vite's resolving mechanism.

Under normal circumstances (when the library is built for Node.js module resolution), they should align.

Once the module is externalized, it cannot be intercepted in any way by Vitest, so any library that imports the same library that you used, will use another module resolution algorithm which might cause a dual package hazard situation. You can debug this using environmental variables: https://github.com/vitest-dev/vitest/tree/main/packages/vite-node#debugging

[Andarist]

Once the module is externalized, it cannot be intercepted in any way by Vitest,

It makes perfect sense.

However, I strongly believe that a dependency loaded by a source file should also be externalized and loaded through node module resolution mechanism. Otherwise, the same dependency is sometimes loaded through Vite and sometimes loaded through node and that leads to very obscure bugs.

[sheremet-va]

However, I strongly believe that a dependency loaded by a source file should also be externalized and loaded through node module resolution mechanism. Otherwise, the same dependency is sometimes loaded through Vite and sometimes loaded through node and that leads to very obscure bugs.

That is harder to achieve (but not impossible). All imports inside the source code are resolved with Vite resolution algorithm first, and then we decide if this particular file should be externalized (to achieve your idea we would need to do the opposite).

There are already options to configure the behavior manually in the meantime: https://vitest.dev/config/#server-deps-external

[Andarist]

Sure, I'm just saying that I think that the current default behavior is not exactly intuitive and that it can lead to obscure bugs. So, if only possible, I'm asking the Vite team to reconsider this default behavior. I'm quite familiar with a lot of existing JS tools and their resolution algorithms etc and what happens here deviates from what I'd call a "quasi-standard". I haven't found this behavior in any other tool. Since this diverges from a common path it makes it harder for me to reason about how my packages might be treated by tooling since it adds yet another column to the matrix of possibilities and knobs (and that matrix already ain't small 😢 )

[sheremet-va]

Sure, I'm just saying that I think that the current default behavior is not exactly intuitive and that it can lead to obscure bugs. So, if only possible, I'm asking the Vite team to reconsider this default behavior. I'm quite familiar with a lot of existing JS tools and their resolution algorithms etc and what happens here deviates from what I'd call a "quasi-standard". I haven't found this behavior in any other tool. Since this diverges from a common path it makes it harder for me to reason about how my packages might be treated by tooling since it adds yet another column to the matrix of possibilities and knobs (and that matrix already ain't small 😢 )

Fully agree that it should work like that 👍🏻

[sheremet-va]

We cannot override Vite's behavior here because it will create more inconsistencies between dev and tests. If the file was processed by Vite, it will go through all plugins, so it can be transformed in a different way. If you find yourself importing different dependencies, try aliasing the dependency to the externalized version.

[Andarist]

We cannot override Vite's behavior here because it will create more inconsistencies between dev and tests. If the file was processed by Vite, it will go through all plugins, so it can be transformed in a different way.

I fully understand the concern but is there any test case that would showcase that actual risk here? I'd love to learn more about specifics related to this particular problem here.

[sheremet-va]

I fully understand the concern but is there any test case that would showcase that actual risk here? I'd love to learn more about specifics related to this particular problem here.

This is more of a general concern. There are a lot of ways in Vite to change how the import is resolved (alias, resolveId/transform/load hooks), so overriding this behavior brings more harm than good in my opinion, - I don't think we can tell the intention to safely change the resolved path of an external module.

[hi-ogawa]

I fully understand the concern but is there any test case that would showcase that actual risk here? I'd love to learn more about specifics related to this particular problem here.

I'm not sure what you exactly mean by "test case" or "actual risk", but I've seen a similar issue happens on Vite SSR, for example with preact vitejs/vite#15503 (comment)

While fiddling with reproduction (hopefully what your imagine as "test case"), I found that import vs require dual package issue can be simply demonstrated on plain NodeJS regardless of whether your main package is esm or commonjs:

https://stackblitz.com/edit/github-ukmpzp?file=repro.cjs
https://stackblitz.com/edit/github-ukmpzp?file=repro.mjs

What actually happens with Vitest (or Vite SSR) is that, your source code's import will all converted to dynamic import (such as await import("effect")), but your cjs dependency doing require("effect") is kept as is, so if your have exports entries, which redirect to a different module, then it will lead to dual package just like the simple reproduction above.

What I found interesting while investigating preact issue is that react doesn't have this issue because both import("react") and require("react") will reach the same cjs module for an obvious reason since react is cjs-only package.