Conditional exports can't be active when key is `browser`

NWYLZW commented 2 months ago

Describe the bug

When I use 'browser' as an optional condition for exporting, I cannot correctly import the module.

Reproduction

https://github.com/NWYLZW/issue-vite-conditional-exports

Steps to reproduce

Run pnpm install followed by pnpm run test

System Info

npx envinfo --system --npmPackages '{vite,@vitejs/*,rollup}' --binaries --browsers

  System:
    OS: macOS 14.1.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 154.44 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.20.4 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.7.0 - /usr/local/bin/npm
    pnpm: 8.12.1 - ~/Library/pnpm/pnpm
  Browsers:
    Safari: 17.1

Used Package Manager

pnpm

Logs

Click to expand!

```shell ~/codes/projects/issue-vite-conditional-exports git:[master] pnpm run test > issue-vite-conditional-exports@1.0.0 test /Users/yijie/codes/projects/issue-vite-conditional-exports > 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. [ 'browser' ] DEV v2.0.5 /Users/yijie/codes/projects/issue-vite-conditional-exports ❯ tests/index.spec.ts (1) ❯ foo (1) × should be true ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ FAIL tests/index.spec.ts > foo > should be true AssertionError: expected 'foo.node' to be 'foo.browser' // Object.is equality Expected: "foo.browser" Received: "foo.node" ❯ tests/index.spec.ts:7:15 5| it('should be true', () => { 6| const f = foo() 7| expect(f).toBe('foo.browser') | ^ 8| expectTypeOf(f).toEqualTypeOf<'foo.browser'>() 9| expectTypeOf(f).not.toEqualTypeOf<'foo.node'>() ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯ Test Files 1 failed (1) Tests 1 failed (1) Start at 18:23:27 Duration 175ms (transform 20ms, setup 0ms, collect 14ms, tests 5ms, environment 0ms, prepare 38ms) FAIL Tests failed. Watching for file changes... ```

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] Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
[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.

NWYLZW commented 2 months ago

The example for reproduction uses Vitest, but in reality, it is not closely related to Vitest; the main issue lies with vite-node.

https://github.com/vitejs/vite/blob/5679d81d91403ea84a329e7076baf0d85a4273a6/packages/vite/src/node/plugins/resolve.ts#L177

https://github.com/vitejs/vite/blob/5679d81d91403ea84a329e7076baf0d85a4273a6/packages/vite/src/node/plugins/resolve.ts#L1113

Based on my debugging results, the handling here seems a bit too tightly coupled with SSR. For other users encountering this issue, you can try setting the ssrTarget parameter to 'webworker'. However, I believe this area should take into account that the user has already actively declared the conditional.

hi-ogawa commented 1 month ago

While I agree Vitest's resolution is confusing at times (other issue I remember is module condition cannot be disabled on Vitest https://github.com/vitejs/vite/issues/16631), is it possible that the exports entries should have browser before node for this specific case? something like:

  "exports": {
    "./foo": {
      "browser": "./src/foo.browser.ts",
      "node": "./src/foo.node.ts"
    }
  },

With this, resolve.exports picks up browser instead of node since internally they set up conditions = ["default", "browser", "import", "node"] and it can match browser entry first https://github.com/lukeed/resolve.exports/blob/294192309b7276bc0913f65a7ee73721611ae383/src/utils.ts#L15-L20

Vitest/Vite automatically adding node condition doesn't seem entirely wrong since it's guarded for ssr usage as you pointed out. The behavior is probably similar to how node --conditions browser -e 'import("issue-vite-conditional-exports/foo")' would resolve exports.

NWYLZW commented 1 month ago

With this, resolve.exports picks up browser instead of node since internally they set up conditions = ["default", "browser", "import", "node"] and it can match browser entry first https://github.com/lukeed/resolve.exports/blob/294192309b7276bc0913f65a7ee73721611ae383/src/utils.ts#L15-L20

I also noticed this point, and it was precisely because of this that I made some modifications to the corresponding browser condition and initiated the related PR.

However, based on the latest code, it seems that the issue here has been exacerbated, as users need a configuration that is used in more places to disable this logic.

vitejs / vite