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

Vitest loads same module twice from dependencies #6494

Closed
6 tasks done
JohnCosta27 opened this issue Sep 13, 2024 · 11 comments · Fixed by #6506
Closed
6 tasks done

Vitest loads same module twice from dependencies #6494

JohnCosta27 opened this issue Sep 13, 2024 · 11 comments · Fixed by #6506
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) vite-node

Comments

@JohnCosta27
Copy link

JohnCosta27 commented Sep 13, 2024

Describe the bug

I found this problem to be reproducible only with Vitest, running Jest or plain node, doesn't seem to cause this issue.

I have two packages. slate and @udecode/slate. @udecode/slate is a small wrapper around slate, and @udecode/slate dependent (through peer dependencies), on slate.

The package slate has some global state (a WeakMap). If you run test cases which mix the use of @udecode/slate and slate, you will get two loads of the same modules from slate, making the state of WeakMap incorrect.

Some other notes.

  • During some attempts at debugging, I wrote console.logs inside the node_modules dist files and saw that slate/dist/index.js was being loaded twice, only in Vitest. Running the same with just node works.
  • We seem to load slate/dist/index.js instead of slate/dist/index.es.js which I would expect to be loaded under these conditions, I have forced vitest to load the esm version, but this causes the same issue, where you simply load the esm version twice, and state is duplicated again.
  • I have tried the resolves.dedup option, and that didn't seem to work either.

Thank you for your help, Vitest has been a great addition to our codebase, and hopefully this issue can make it a little bit better.

Reproduction

This demonstrates that two different versions of the same module are correct, shown inside the test folder.

I have also provided another file called test.node.mjs which has the same test cases, but just outputs them to stdout. In this case the results are different from vitest, and what we expect.

https://stackblitz.com/edit/vitest-dev-vitest-dkrj52?file=test%2Fbasic.test.ts&initialPath=__vitest__/

System Info

System:
    OS: Linux 6.10 Garuda Linux
    CPU: (12) x64 Intel(R) Core(TM) i7-8700K CPU @ 3.70GHz
    Memory: 15.39 GB / 31.29 GB
    Container: Yes
    Shell: 3.7.1 - /bin/fish
  Binaries:
    Node: 22.8.0 - /usr/bin/node
    Yarn: 1.22.22 - /usr/bin/yarn
    npm: 10.8.3 - /usr/bin/npm
    pnpm: 9.7.1 - /usr/bin/pnpm
  npmPackages:
    @vitest/ui: ^2.1.0 => 2.1.0
    vite: ^5.4.5 => 5.4.5
    vitest: ^2.1.0 => 2.1.0

Used Package Manager

yarn

Validations

@JohnCosta27 JohnCosta27 changed the title Vitest loads same module twice with from dependencies Vitest loads same module twice from dependencies Sep 13, 2024
@hi-ogawa
Copy link
Contributor

hi-ogawa commented Sep 14, 2024

Not sure what's tripping up Vitest's resolution, but it looks like slate is not properly externalized and that's causing double modules (direct import slate is processed by Vitest while indirect import of slate from @udecode/slate is loaded directly on Node). You see something like this when running with DEBUG:

$ DEBUG=vite-node:* npm test
...
  vite-node:client:native /home/hiroshi/Downloads/vitest-dev-vitest-dkrj52/node_modules/@udecode/slate/dist/index.mjs +16ms
  vite-node:server:request /home/hiroshi/Downloads/vitest-dev-vitest-dkrj52/node_modules/slate/dist/index.js +26ms
  vite-node:client:execute /home/hiroshi/Downloads/vitest-dev-vitest-dkrj52/node_modules/slate/dist/index.js +164ms
....

Potential workaround is to use test.server.deps.external: ["slate"] to avoid Vitest processing slate like this:
https://stackblitz.com/edit/vitest-dev-vitest-quwptx?file=vite.config.ts

export default defineConfig({
  test: {
    server: {
      deps: {
        external: ['slate']
      }
    }
  },
})

@12joan
Copy link

12joan commented Sep 14, 2024

@hi-ogawa Thanks for the this! Do you know if there's a way we can fix this problem in the slate package?

@hi-ogawa
Copy link
Contributor

Thanks for the this! Do you know if there's a way we can fix this problem in the slate package?

I think this could be a bug of Vitest. There's some heuristics to decide for vite-node to whether process an external package or not and probably slate is hitting some edge cases here. Let me check a bit:

// The code from https://github.com/unjs/mlly/blob/c5bcca0cda175921344fd6de1bc0c499e73e5dac/src/syntax.ts#L51-L98
async function isValidNodeImport(id: string) {
const extension = extname(id)
if (BUILTIN_EXTENSIONS.has(extension)) {
return true
}
if (extension !== '.js') {
return false
}
id = id.replace('file:///', '')
const package_ = await findNearestPackageData(dirname(id))
if (package_.type === 'module') {
return true
}
if (/\.(?:\w+-)?esm?(?:-\w+)?\.js$|\/esm?\//.test(id)) {
return false
}
const code = await fsp.readFile(id, 'utf8').catch(() => '')
return !ESM_SYNTAX_RE.test(code)
}

@12joan
Copy link

12joan commented Sep 14, 2024

Could it be because Slate using the extension .es.js for its module export? I don't see that extension listed anywhere in Vitest's code.

https://github.com/ianstormtaylor/slate/blob/main/packages/slate/package.json#L7-L11

@hi-ogawa
Copy link
Contributor

It looks like ESM_SYNTAX_RE is false-detecting a code inside comments.

$ node
> const ESM_SYNTAX_RE = /(?:[\s;]|^)(?:import[\s\w*,{}]*from|import\s*["'*{]|export\b\s*(?:[*{]|default|class|type|function|const|var|let|async function)|import\.meta\b)/m;
undefined
> code = `// import { thisIsComment } from "test"`
'// import { thisIsComment } from "test"'
> ESM_SYNTAX_RE.test(code)
true

This is already fixed on mlly unjs/mlly#196, so we can probably copy the same thing to vite-node.

@hi-ogawa hi-ogawa added vite-node p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Sep 14, 2024
@hi-ogawa
Copy link
Contributor

It looks like mlly's regex /\/\*.+?\*\/|\/\/.*(?=[nr])/g in unjs/mlly#196 doesn't strip multi line jsdoc comment, which is used by slate https://github.com/ianstormtaylor/slate/blob/85a1e1d3f3b9c47144a5a3b299989a088b45f5b4/packages/slate/src/interfaces/scrubber.ts#L18

Not sure whether we should go with regex or maybe we can use legitimate tokenizer like https://github.com/guybedford/es-module-lexer or https://github.com/antfu/strip-literal

@12joan
Copy link

12joan commented Sep 14, 2024

I think \/\*.*?\*\/gs should work to target multi-line comments. The ? in .*? makes it lazy so it doesn't match multiple comments at once, and the s flag at the end tells . to match newline characters.

@12joan
Copy link

12joan commented Sep 14, 2024

@hi-ogawa
Copy link
Contributor

Yeah, dotall flag s should help this case, but I wasn't entirely sure how much we can trust regex in general. I asked chatgpt and also checked stackoverflow https://stackoverflow.com/questions/5989315/regex-for-match-replacing-javascript-comments-both-multiline-and-inline and they are all slightly different and I was confused.

Using es-module-lexer would be fool-proof, but perf impact is the concern.

@12joan
Copy link

12joan commented Sep 14, 2024

I think regex is probably good enough for the purpose of detecting whether or not the code contains imports. Even if we accidentally strip out some code that looks like a comment but isn't, there's a good chance that the remaining code will still tell us whether we're using ESM or not.

If the goal was to get a definite list of imports, using a lexer would be a much better solution, but I'm not sure it's necessary here.

@JohnCosta27
Copy link
Author

Thanks @hi-ogawa, this has been very helpful

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

Successfully merging a pull request may close this issue.

3 participants