Import inconsistency between vitest and vite breaks test runs

tirithen commented 7 months ago

Describe the bug

The vitest package cannot always build the same code that vite can build. Specifically the errors seem to be around the imports in various combinations of ESM/CJS and module/main properties in package.json.

Expected behavior:

Any code that builds with the command $ vite build should also be possible to test with the command $ vitest as there are build instructions in the vite.config.js file.

Actual behaviour:

Running $ vitest gives the error:

Error: Failed to resolve entry for package "@myorg/mypackage". The package may have incorrect main/module/exports specified in its package.json.

There has been issues reported around this kind of error several times, with suggested workarounds on adding configuration in vite.config.js in the style of:

  test: {
    deps: {
      inline: ['@myorg/mypackage'],
    },
  }

// or in other cases

export default {
  resolve: {
    mainFields: ['module']
  }
}

This workaround is not ideal as the vite.config.js was already building correctly without these workarounds running $ vite build. Relying on extra configuration risks going back to the webpack days where a huge number of configuration parameters was needed to build anything at all. For newer build/test tools to give any benefit from the old, they basically must reduce the amount of specific config by finding sane defaults.

In the real world lives in a mixture of ESM and CJS, and in some cases odd hybrids for some more more years to come, and the build and test tools therefore must support these scenarios. No one will me more happy than me once every npm package uses strictly plain buildless ESM.

My suggestion as vitest is related to the vite project, is that it default should re-use the vite build code to ensure interoperability between the two packages. This is the expectation that every new user will have as the names are so closely related.

Here are some related issues that suffers from these ESM/CJS issues, several of which never lead to any actual code changes in vitest, but instead was suggested the workaround mentioned:

This history of issues around the same area would in my opinion show even more that the defaults for vitest is expected to be that a project with a vite.config.js config that can be run built with $ vite build should also be possible to test with $ vitest. When that expectation is not met it leads to confusion, and eventually to more issues being reported here.

Some automated tests could be created for these the scenarios in the issues linked above to ensure this keeps working.

All this being said I do know about the complexities around getting everything right, expecially when ESM/CJS is mixed in the same project. The great news is that vite build tool has solved a lot of this for us, so if that exact code can also be leveraged here that should ensure the compatibility.

Also it is better to be correct than having fast runtimes by default, I saw in one issue ("Yes, by default Vitest doesn't respect module field for performance reasons:") that main (CJS) is used over module (ESM) by default? If this is the case, probably that choice is part of the problem. Remember that when tests run in CI environments, while it is nice when things are fast, it is even slower to have something fast break and then requiring developers to spend hours debugging vite/build configurations. It is better to be correct by default.

What are your takes around this from the project's perspective?

Reproduction

Visit reproduction repo at https://github.com/tirithen/vitest-import-error or

Clone repo git clone git@github.com:tirithen/vitest-import-error.git
$ cd vitest-import-server
$ npm install
$ npx vite build
$ npx vitest

Step 4 builds as expected (produces output in dist/), step 5 crashes with the following error:

npx vitest    
The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.

 DEV  v1.4.0 /home/user/Projects/vitest-import-error

 ❯ index.test.js (0)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Suites 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  index.test.js [ index.test.js ]
Error: Failed to resolve entry for package "@jdlien/validator". The package may have incorrect main/module/exports specified in its package.json.
 ❯ packageEntryFailure node_modules/vite/dist/node/chunks/dep-whKeNLxG.js:48302:17
 ❯ resolvePackageEntry node_modules/vite/dist/node/chunks/dep-whKeNLxG.js:48299:5
 ❯ tryNodeResolve node_modules/vite/dist/node/chunks/dep-whKeNLxG.js:48069:20
 ❯ Context.resolveId node_modules/vite/dist/node/chunks/dep-whKeNLxG.js:47819:28
 ❯ Object.resolveId node_modules/vite/dist/node/chunks/dep-whKeNLxG.js:51096:64
 ❯ TransformContext.resolve node_modules/vite/dist/node/chunks/dep-whKeNLxG.js:50787:23
 ❯ normalizeUrl node_modules/vite/dist/node/chunks/dep-whKeNLxG.js:66083:34
 ❯ async file:/home/user/Projects/vitest-import-error/node_modules/vite/dist/node/chunks/dep-whKeNLxG.js:66247:47
 ❯ TransformContext.transform node_modules/vite/dist/node/chunks/dep-whKeNLxG.js:66168:13

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

 Test Files  1 failed (1)
      Tests  no tests
   Start at  14:15:33
   Duration  296ms (transform 20ms, setup 0ms, collect 0ms, tests 0ms, environment 0ms, prepare 66ms)

 FAIL  Tests failed. Watching for file changes...
       press h to show help, press q to quit
Cancelling test run. Press CTRL+c again to exit forcefully.

System Info

System:
    OS: Linux 6.8 Arch Linux
    CPU: (8) x64 Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
    Memory: 4.33 GB / 7.64 GB
    Container: Yes
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 21.6.1 - ~/.nvm/versions/node/v21.6.1/bin/node
    Yarn: 1.22.22 - /usr/bin/yarn
    npm: 10.2.4 - ~/.nvm/versions/node/v21.6.1/bin/npm
  Browsers:
    Chromium: 123.0.6312.105
  npmPackages:
    @vitejs/plugin-vue: ^5.0.4 => 5.0.4 
    @vitest/coverage-v8: ^1.4.0 => 1.4.0 
    vite: 5.2.7 => 5.2.7 
    vitest: ^1.4.0 => 1.4.0

Used Package Manager

npm

Validations

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

sheremet-va commented 7 months ago

My suggestion as vitest is related to the vite project, is that it default should re-use the vite build code to ensure interoperability between the two packages. This is the expectation that every new user will have as the names are so closely related.

We cannot use vite build because a lot of our API won't work (like module mocking) because the built code doesn't have the concept of a module graph.

The great news is that vite build tool has solved a lot of this for us, so if that exact code can also be leveraged here that should ensure the compatibility.

Vite has solved it in the browser with an optimizer, the same cannot be reimplemented for tests running in Node.js. For example, Vite will reload the page if during dependency crawling it finds a duplicate - we cannot do the same in tests (it can also reload the page during a dynamic import which will break your test coverage/results). We also cannot apply the same mechanism to every package running in Node.js as some of them are test-only and expect to be running in Node, and not be processed by the browser optimizer (for example, react package cannot be optimized because it might be used by @testing-library/react which uses Node.js APIs - so we can't transform it like Vite does). Vitest provides a way to enable this optimizer via a deps.optimizer option. It has been disabled by default since 1.2 (I think) because it introduces even more problems.

Also it is better to be correct than having fast runtimes by default, I saw in one issue ("Yes, by default Vitest doesn't respect module field for performance reasons:") that main (CJS) is used over module (ESM) by default? If this is the case, probably that choice is part of the problem. Remember that when tests run in CI environments, while it is nice when things are fast, it is even slower to have something fast break and then requiring developers to spend hours debugging vite/build configurations. It is better to be correct by default.

The problem is people wanting to run tests for the browser environment in Node.js, and not that we don't respect invalid Node.js package.json configuration. Our choice follows the spec of a runtime that your code is running in. If your application is running in the browser, run tests in the browser - there is nothing that we can enable to satisfy every user.

What are your takes around this from the project's perspective?

We are planning to explore Node.js Loader API when it is stabilized to improve the story with inline/external packages.

In the meantime, you should use workarounds or run tests in the browser to help us improve the browser mode.

tirithen commented 7 months ago

First of all, thank you for taking the time to answer! Also thank you for explaining details around the current situation!

No doubt it is hard to finding sane defaults that autodetects the runtime environment and adapts perfectly to every situation. As users we surely see the surface APIs rather than all the underlying challenges.

Making tools that works for everyone is hard, I also know that from experience, and I appreciate this project and I really think we need simpler test tooling (especially with less configuration). I still want to explain a bit around how I think the naming and documentation might confuse its users and why some clarification might be good to add to the documentation:

I think the main confusion as a user of the package is the naming confusion with the vite package.

Vite uses the tagline "Next Generation Frontend Tooling"
Vitest uses the tagline "Next generation testing framework powered by Vite."

Also the names are obviously related, and that they are meant to be interoperable is also the impression that I get when reading the why on your web page https://vitest.dev/guide/why.html . The two also shares the same vite.config.js at least to some extent. That I think sets the expectation that vitest would run the same as vite. It also sets the expectation that this tool will be focused on running tests for browser code rather than for Node.js code.

My personal opinion and assumption as a result of this is that it would be great if that is where the focus of this tool was aimed at, and that any Node.js user trying to use this package would need to add custom configuration, code, or command line arguments (like --node or similar). The related package Vite is after all is a front end tool. This is of course just an opinion from my user perspective.

Thank you also for coming back with suggestions around browser mode and deps.optimizer, I'll have a look at them and compare that against instead using something more specifically front end focused like Web Test Runner or Playwright and see which matches the my use cases better.

Thanks for working on tooling that can help bring sanity to all the configuration/setups/transpilation steps currently needed to run and test a JavaScript project!

vitest-dev / vitest