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 doesn't honor resolve conditions for nested modules in workspace. #5301

Open
6 tasks done
DylanPiercey opened this issue Feb 26, 2024 · 10 comments
Open
6 tasks done
Labels
feat: workspace Issues and PRs related to the workspace feature pending triage

Comments

@DylanPiercey
Copy link
Contributor

DylanPiercey commented Feb 26, 2024

Describe the bug

resolve.conditions config for workspace is ignored when resolving nested node_module.

When you import a module, which then imports another module that relies on export conditions vitest is not honoring those conditions if they are defined in the workspace config.

If you define the resolve conditions in the main vitest.config.ts it will resolve the nested module, however I am trying to have server & client tests in a workspace which need different conditions (the browser condition is different between them).

Note this previously worked fine in 1.2.2. It was broken in 1.3.0.

Reproduction

Clone https://github.com/DylanPiercey/vitest-resolve-condition-nested-issue

System Info

System:
    OS: macOS 14.3.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 7.14 GB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.9.0 - ~/Library/Caches/fnm_multishells/47552_1708985680346/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 10.2.5 - ~/Library/Caches/fnm_multishells/47552_1708985680346/bin/npm
    pnpm: 8.15.1 - ~/pnpm/pnpm
    bun: 1.0.25 - ~/.bun/bin/bun
  Browsers:
    Chrome: 122.0.6261.69
    Edge: 122.0.2365.52
    Safari: 17.3.1
  npmPackages:
    @vitest/ui: latest => 1.3.1 
    vite: latest => 5.1.4 
    vitest: latest => 1.3.1

Used Package Manager

npm

Validations

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 27, 2024

Note this previously worked fine in 1.2.2. It was broken in 1.3.0.

Thanks for the reproduction. When I test with Vitest 1.2.2, it looks like I get the same error as 1.3.0:

$ envinfo --npmPackages

  npmPackages:
    @vitest/ui: 1.2.2 => 1.2.2 
    vite: 5.1.4 => 5.1.4 
    vitest: 1.2.2 => 1.2.2 
npm run test
 DEV  v1.2.2 /home/hiroshi/code/tmp/vitest-5301-workspace-resolve

 ✓ |server| test/server.test.ts (1)
 ❯ |browser| test/browser.test.ts (1)
   × is browser code

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  |browser| test/browser.test.ts > is browser code
AssertionError: expected 'server' to equal 'browser'

- Expected
+ Received

- browser
+ server

 ❯ test/browser.test.ts:5:15
      3| 
      4| test('is browser code', () => {
      5|   expect(env).equals('browser');
       |               ^
      6| });
      7| 

Is it possible that you also upgraded Vite at the same time? It would be great if you can identify previous working versions of both Vite and Vitest.

There was a change related to resolve.conditions in Vitest 1.2.1 #4980, but from a quick look, it doesn't seem related to your workspace issue.

Also one more change related to "browser mode" testing and workspace in Vitest 1.2.1 #4947. Do you happen to use this though your reproduction didn't include this setup?

Regardless of whether it was working before, it looks like a bug to me. Nonetheless, it's good to know if it is regression or not.

@hi-ogawa hi-ogawa added the feat: workspace Issues and PRs related to the workspace feature label Feb 27, 2024
@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 27, 2024

As far as I understand, it doesn't look like Vitest can handle resolve.conditions per workspace project by the same reason as test.env issue #5016 (unless it uses browser mode).

Currently execArgv with --conditions node etc... is created based on main Vite dev server config:

const potentialConditions = new Set(['production', 'development', ...ctx.server.config.resolve.conditions])

and this is passed to new Tinypool constructor and shared between all thread/fork workers. Currently Tinypool instance is not aware of per-workspace-project configuration.

@DylanPiercey
Copy link
Contributor Author

DylanPiercey commented Feb 27, 2024

@hi-ogawa I just tested in my reproduction with many versions of vitest (and vite 5) and I'm not sure that particular example ever worked, although ideally it would.

However the main thing that had me trying to create a reproduction is because of an issue we're seeing in the Marko ecosystem which I thought I had isolated via the above. In Marko it has a similar package.json condition remap that was honored in vitest@1.2.2 but not in 1.3.0+.

I created a reproduction (although less simplified since this is actually running Marko code) https://github.com/DylanPiercey/vitest-issue-marko

In the above example vitest loads node_modules/marko/src/node_modules/@internal/components-registry/index.js when it should be loading node_modules/marko/src/node_modules/@internal/components-registry/index-browser.js since there is an export condition remap.

Again the above works as-is in 1.2.2 but breaks when I upgrade just vitest to 1.3.0 🤔

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 27, 2024

Thanks for the update. Now I see that it works on 1.2.2, but it looks like it's working even without resolve.conditions (I just commented out). From my understanding, resolve.conditions in workspace config wouldn't have worked before, so that matches with my expectation. Can you confirm?

If resolve.conditions is irrelevant, then the regression might be somewhere else. I'll take a closer look at your marko reproduction.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 27, 2024

Okay, I remembered one more potentially relevant change

and indeed it looks like deps.optimizer.web.enabled: true is now necessary for your marko setup:
https://stackblitz.com/edit/github-japroc?file=vitest.workspace.ts

Ah, wait..., but I'm confused that it's working when it's not using vitest.workspace, so I might be still missing something. (nvm, it works without optimizer because --conditions browser is correctly injected for non-workspace case)

@DylanPiercey
Copy link
Contributor Author

@hi-ogawa I really appreciate the digging here!

I apologize, the config there is misleading because the @marko/vite plugin actually automatically adds the resolve.conditions when running in vitest dom environment (see https://github.com/marko-js/vite/blob/main/src/index.ts#L244-L256).

I think I will just update the above code to also enable the deps.optimizer.web.enabled config.

@DylanPiercey
Copy link
Contributor Author

Dang, I was premature. My patch in marko-js/vite#113 doesn't work. Apparently setting this in the config hook is too late?

@DylanPiercey
Copy link
Contributor Author

Forget me. I'm just dumb and made a typo 😓. I should probably get vitest itself in the @marko/vite test suite...

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 27, 2024

@DylanPiercey Thanks for verifying the workaround on Marko. Many things were intertwined in this particular issue, but for Vitest side, I think the main issue/limitation is about resolve.conditions is not applied for workspace project.

I'll update the issue title accordingly and track it as an known issue. I cannot immediately see how Vitest would support this, but maybe it might be possible in the future since tests between workspace project are guaranteed to be isolated so setting execArgv with --conditions ... separately sounds technically possible.

@hi-ogawa hi-ogawa changed the title Vitest no longer honors resolve conditions for nested modules in workspace. Vitest doesn't honor resolve conditions for nested modules in workspace. Feb 27, 2024
@DylanPiercey
Copy link
Contributor Author

Sounds good, thanks for your help @hi-ogawa!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: workspace Issues and PRs related to the workspace feature pending triage
Projects
None yet
Development

No branches or pull requests

2 participants