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

Vite 6 compatibility issue #7070

Open
6 tasks done
thebanjomatic opened this issue Dec 11, 2024 · 6 comments · May be fixed by #7071
Open
6 tasks done

Vite 6 compatibility issue #7070

thebanjomatic opened this issue Dec 11, 2024 · 6 comments · May be fixed by #7071

Comments

@thebanjomatic
Copy link
Contributor

thebanjomatic commented Dec 11, 2024

Describe the bug

When using a browser-based evironment (happy-dom, jsdom, browser), vitest still passes conditions: ['node'] in the resolve config.

When using Vite 5, this doesn't really matter because it would infer that we were targetting a web environment and it would resolve in that context anyway. I'm not 100% sure on how this worked before, but tryNodeResolve was being passed a targetWeb boolean and the returned ID will match the browser / default import conditions instead of the node condition.

With Vite 6 a lot of this default environment magic was stripped out as part of the Environments api refactor, and now it's just using the resolve.conditions from the corresponding client/ssr configs. This results in a change in behavior where Vitest now uses the node condition for browser environments when using Vite 6.

Using the node condition instead of browser can result in test failures due to differences in behavior between browser and node implementations of a library, but I've also run into issues where esbuild as part of deps optimization generates invalid code where the module itself is "babel compatible" but tries to deconstruct the named exports from import_mod1.default instead of import_mod1.

These issues reproduce in the latest Vitest 2.1.8 using a resolution for Vite 6.0.3, as well as Vitest 3.0.0-beta.2.

Reproduction

I'm still working on creating a reproducer that I can share and should be able to do so by the end of the day.

In terms of the actual code changes, I think these two locations can be changed to use:

conditions: [testConfig.environment === 'node' ? 'node' : 'browser']

resolve: {
// by default Vite resolves `module` field, which not always a native ESM module
// setting this option can bypass that and fallback to cjs version
mainFields: [],
alias: testConfig.alias,
conditions: ['node'],
},

resolve: {
// by default Vite resolves `module` field, which not always a native ESM module
// setting this option can bypass that and fallback to cjs version
mainFields: [],
alias: testConfig.alias,
conditions: ['node'],
},

But it's also a little unclear to me if we should be clearing out the mainFields: [], for the browser-based environments also.

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (20) x64 12th Gen Intel(R) Core(TM) i7-12800H
    Memory: 11.40 GB / 31.64 GB
  Binaries:
    Node: 20.18.0 - ~\AppData\Local\fnm_multishells\18680_1733347780000\node.EXE
    Yarn: 4.5.3 - ~\AppData\Local\fnm_multishells\18680_1733347780000\yarn.CMD
    npm: 10.9.1 - D:\source\delete-me\test-vite6\node_modules\.bin\npm.CMD
    bun: 1.1.29 - ~\.bun\bin\bun.EXE
  Browsers:
    Edge: Chromium (127.0.2651.74)
    Internet Explorer: 11.0.22621.3527
  npmPackages:
    vite: ^6.0.0 => 6.0.3

Used Package Manager

yarn

Validations

@sheremet-va
Copy link
Member

There is no compatibility issue here. happy-dom and jsdom tests are running in Node.js with browser globals. There is no widespread decision on how the browser condition should be treated - some libraries expect imports to follow bundlers logic (don't use extensions) which breaks in Vitest when externalized, some just expect globals to be present. Vitest added these conditions for a reason and we are keeping them. The same can be said about the mainFields treatment - since Node.js can't resolve those, it's better to remove them altogether to avoid module hazard if the module is externalized.

When using a browser-based evironment (happy-dom, jsdom, browser)

The browser mode doesn't modify resolve conditions.

@sheremet-va
Copy link
Member

This results in a change in behavior where Vitest now uses the node condition for browser environments when using Vite 6.

I missed this part - we might need to inject default conditions now in the config, but we shouldn't remove the node condition.

@thebanjomatic
Copy link
Contributor Author

thebanjomatic commented Dec 11, 2024

Reproducer:
https://stackblitz.com/edit/vitest-dev-vitest-xzepei6n?file=package.json&view=editor

Vite 6.0.3:
image

Vite 5.4.9:
image

So, it does look like the node conditions were still used in the vite 5 case as that test case fails in both versions, but the tslib error when using optimized deps only reproduces in vite 6 since in vite 5, since the "vite:resolve" plugin was going down a different path (ssr: false implies targetWeb: true) and returning the browser condition instead of node.

This tslib issue is the failure that drove me down this path, and ultimately what is blocking my teams from adopting Vite 6 at this time. If that issue should be filed with the Vite repo instead, let me know.

@sheremet-va
Copy link
Member

Reproducer: stackblitz.com/edit/vitest-dev-vitest-xzepei6n?file=package.json&view=editor

I just want to note that "browser" here is the "happy-dom" environment, not the actual browser, the browser mode in Vitest uses a separate Vite server.

@thebanjomatic
Copy link
Contributor Author

thebanjomatic commented Dec 11, 2024

Yes that's correct. I probably should have been more clear in the naming of the project. I think that's fine and I mainly want things to continue working the same as they do in Vite 5. Since there is no intention of changing away from conditions: ["node"], I have updated the reproducer to reflect that and to instead only focus on the failure case:

https://stackblitz.com/edit/vitest-dev-vitest-xzepei6n?file=package.json

Vite 5:
image

Vite 6:
image

@thebanjomatic
Copy link
Contributor Author

From testing, it looks like adding back in the default conditions for vite >=6 fixes this issue. I'll submit a PR.

thebanjomatic pushed a commit to thebanjomatic/vitest that referenced this issue Dec 11, 2024
Vite 6 no longer applies default conditions when you override resolve.conditions.
This PR adds them back conditionally based on the vite version.

Fixes vitest-dev#7070
thebanjomatic pushed a commit to thebanjomatic/vitest that referenced this issue Dec 11, 2024
Vite 6 no longer applies default conditions when you override resolve.conditions.
This PR adds them back conditionally based on the vite version.

Fixes vitest-dev#7070
thebanjomatic pushed a commit to thebanjomatic/vitest that referenced this issue Dec 11, 2024
Vite 6 no longer applies default conditions when you override resolve.conditions.
This PR adds them back conditionally based on the vite version.

Fixes vitest-dev#7070
thebanjomatic pushed a commit to thebanjomatic/vitest that referenced this issue Dec 11, 2024
Vite 6 no longer applies default conditions when you override resolve.conditions.
This PR adds them back conditionally based on the vite version.

Fixes vitest-dev#7070
@thebanjomatic thebanjomatic linked a pull request Dec 11, 2024 that will close this issue
6 tasks
thebanjomatic pushed a commit to thebanjomatic/vitest that referenced this issue Dec 11, 2024
Vite 6 no longer applies default conditions when you override resolve.conditions.
This PR adds them back conditionally based on the vite version.

Fixes vitest-dev#7070
thebanjomatic pushed a commit to thebanjomatic/vitest that referenced this issue Dec 11, 2024
Vite 6 no longer applies default conditions when you override resolve.conditions.
This PR adds them back conditionally based on the vite version.

Fixes vitest-dev#7070
thebanjomatic pushed a commit to thebanjomatic/vitest that referenced this issue Dec 11, 2024
Vite 6 no longer applies default conditions when you override resolve.conditions.
This PR adds them back conditionally based on the vite version.

Fixes vitest-dev#7070
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants