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

resolve.conditions not working reliably on custom environment #18222

Closed
7 tasks done
hi-ogawa opened this issue Sep 28, 2024 · 3 comments · Fixed by #18302
Closed
7 tasks done

resolve.conditions not working reliably on custom environment #18222

hi-ogawa opened this issue Sep 28, 2024 · 3 comments · Fixed by #18302
Labels
feat: environment API Vite Environment API p3-minor-bug An edge case that only affects very specific usage (priority)
Milestone

Comments

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Sep 28, 2024

Describe the bug

I'm testing a custom environment like:

  environments: {
    worker: {
      webCompatible: true,
      resolve: {
        conditions: ["worker"],
        noExternal: true,
      },
    },

and a package like:

{
  "name": "test-dep-conditions",
  "private": true,
  "type": "module",
  "exports": {
    ".": {
      "worker": "./index.worker.js",
      "browser": "./index.browser.js",
      "default": "./index.default.js"
    }
  }
}

When importing such package both from default client environment and custom environment, custom environment side picks up ./index.browser.js in some cases.

Reproduction

https://github.com/hi-ogawa/reproductions/tree/main/vite-environment-resolve-condition

Steps to reproduce

  • open https://stackblitz.com/github/hi-ogawa/reproductions/tree/main/vite-environment-resolve-condition?file=src%2Fentry.tsx
    • shows client: index.browser.js correctly
  • click /worker link
    • shows worker: index.browser.js but it's expected to show worker: index.worker.js
  • go back and click /custom1 link
    • shows custom1: index.browser.js but it's expected to show custom1: index.custom1.js
  • kill server (Control+C) and restart
  • click /worker link again
    • now it shows worker: index.worker.js
  • go back and click /custom1 link
    • shows custom1: index.worker.js but it's expected to show custom1: index.custom1.js

In this reproduction, I added three extra custom environments with and without webCompatible: true. It looks like the one with webCompatible has some mixed up resolutions depending on the order how they are resolved. It looks like resolution is cached by webCompatible, so for each true/false case, resolution depends on the order how they are resolved.

I also tested with the latest https://pkg.pr.new/vite@95020ab but the behavior seems to be same.

System Info

(stackblitz)

  System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    vite: 6.0.0-beta.1 => 6.0.0-beta.1

Used Package Manager

npm

Logs

No response

Validations

@hi-ogawa hi-ogawa changed the title resolve.conditions not working reliably on custom environment resolve.conditions not working reliably on custom environment with webCompatible: true Sep 28, 2024
@hi-ogawa hi-ogawa changed the title resolve.conditions not working reliably on custom environment with webCompatible: true resolve.conditions not working reliably on custom environment Sep 28, 2024
@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Sep 28, 2024

It looks like the issue is setResolvedCache/getResolvedCache's cache key relying on webCompatible:

setResolvedCache: (key: string, entry: string, targetWeb: boolean) => void
getResolvedCache: (key: string, targetWeb: boolean) => string | undefined

Similar issue about the fact that cache key is too coarse is discussed in #12957 (comment)

@patak-dev
Copy link
Member

patak-dev commented Sep 28, 2024

This was broken before too then, in case ssr.resolve.conditions was different. setResolvedCache and getResolvedCache should be per environment, and not only keyed against webCompatible, no?

@patak-dev patak-dev added this to the 6.0 milestone Sep 28, 2024
@patak-dev patak-dev added p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Sep 28, 2024
@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Sep 28, 2024

This was broken before too then, in case ssr.resolve.conditions was different.

Probably so but maybe only when ssr.target: 'webworker' which makes both client and ssr targetWeb: true.

setResolvedCache and getResolvedCache should be per environment, and not only keyed against webCompatible, no?

I haven't digested the other issue #12957 (comment) but it looks like per-environment is not enough since there might be a case where single environment should resolve differently based on isRequire etc...


EDIT: I checked Vite 5 and indeed the issue is the same but only when ssr.target: 'webworker' https://github.com/hi-ogawa/reproductions/blob/main/vite-environment-resolve-condition/repro-v5.js

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: environment API Vite Environment API p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants