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

package.json conditional exports in dependencies resolve incorrectly #4971

Closed
6 tasks done
justin-schroeder opened this issue Jan 16, 2024 · 6 comments · Fixed by #4980
Closed
6 tasks done

package.json conditional exports in dependencies resolve incorrectly #4971

justin-schroeder opened this issue Jan 16, 2024 · 6 comments · Fixed by #4980
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)

Comments

@justin-schroeder
Copy link

justin-schroeder commented Jan 16, 2024

Describe the bug

👋 First thank you for saving the testing world 😂

I’m a library maintainer (FormKit) and we are gearing up to ship development versions of our packages (for improved error messages etc). We do this by specifying in the package.json’s exports a conditional export for "development". This works great in Vite providing development versions of the package in dev, and production ones when built for prod.

However, Vitest appears to be inconsistent in which module is resolved even when running with an explicit NODE_ENV of development or an config.test.env.NODE_ENV of development. This leads to errors where module scoped state is not consistent.

It appears that vite’s *.spec.ts files resolve the development version properly and the initial dependency in the resolution chain is correct too, but sub-dependencies resolve the production version only. I thought this may have something to do with threads, but have been unable to validate that.

Reproducing this can be a bit tricky so I created, and have provided, 2 packages with the following structure:

Vitest Conditional Import Reproduction (vcir)
|--vcir-package-b
|----vcir-package-a

Each of these packages has a package.json that includes an exports block something like this:

{
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "import": {
        "types": "./dist/index.d.mts",
        "development": "./dist/index.dev.mjs",
        "default": "./dist/index.mjs"
      },
      "require": {
        "types": "./dist/index.d.ts",
        "default": "./dist/index.js"
      }
    }
  }
}

Package A in this scenario has a module scoped variable that gets incremented by the exported count method. Package B contains a Vue component that calls count from Packge A and then renders the value in a Vue component. Every time the component is rendered the value should increment. This works great in the when running vite, but fails in vitest. Using the debugger you can see that the vcir-package-a uses both index.mjs and index.mjs

Reproduction

Steps to reproduce this:

  1. Clone: https://github.com/justin-schroeder/vitest-conditional-import-reproduction
  2. Install dependencies
  3. Validate this works by running npm run dev (notice the count should start at 2 because count() is called manually in the setup function)
  4. Run Vitest and observe the same thing fails in vitest.

System Info

System:
    OS: macOS 13.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 118.55 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.17.1 - /usr/local/bin/node
    Yarn: 1.22.18 - ~/.yarn/bin/yarn
    npm: 9.6.7 - /usr/local/bin/npm
    pnpm: 8.11.0 - ~/Library/pnpm/pnpm
    bun: 1.0.1 - ~/.bun/bin/bun
  Browsers:
    Brave Browser: 120.1.61.116
    Chrome: 120.0.6099.216
    Edge: 120.0.2210.133
    Safari: 16.2

Used Package Manager

pnpm

Validations

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Jan 16, 2024

I experimented with your reproduction and I think the issue is somewhat related to the fact about "source files" and "external files" explained in #4365 (comment)

The resolution discrepancy occurs with development exports condition because NodeJS actually requires users to explicitly provide --conditions=development to enable this, but on the other hand, Vite (vite-node) automatically handles this based on dev/prod mode as a bundler feature https://vitejs.dev/config/shared-options.html#resolve-conditions.

To mitigate this issue currently on Vitest, it's necessary to explicitly set --conditions via poolOptions.execArgv or resolve.conditions. It's also possible to get away from this by treating vcir-package-b as "source files" via server.deps.inline. I updated your reproduction to illustrate this:

https://stackblitz.com/edit/github-b8xjmk?file=vitest.config.ts

export default defineConfig({
  test: {
    environment: 'jsdom',

    // this
    poolOptions: {
      threads: {
        execArgv: ['--conditions=development'],
      },
    },

    // or this
    server: {
      deps: {
        inline: ['vcir-package-b'],
      },
    },
  },
  // or this (essentially same as 1st one since https://github.com/vitest-dev/vitest/blob/42be8249eac3cadcfaed41f20e2a0147ccb4785d/packages/vitest/src/node/pool.ts#L63)
  resolve: {
    conditions: process.env.VITEST ? ['development'] : undefined,
  },
});

I feel either approach is still clumsy (if you're a library author and needs to ask users to do this), but asking Vitest to automatically inject --conditions=development to align with Vite also feels too much. Maybe I'm missing something and someone might have a better idea about this.

@justin-schroeder
Copy link
Author

Thank you for the pointers on how to resolve this (can confirm poolOptions.threads.execArgv works great) — it’s very helpful to understand what is going on there.

I’m certainly no expert on the internals of Vitest or its thread pooling, but as I heavy user I suppose my expectation is that Vitest always attempts to recreate Vite’s environment as accurately as possible (reusing it where possible). It would be ideal (I think?) if Vitest used the same conditionals that vite was using.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Jan 16, 2024

as I heavy user I suppose my expectation is that Vitest always attempts to recreate Vite’s environment as accurately as possible (reusing it where possible).

I totally see that point as well and I was thinking maybe Vitest can replicate some code from Vite https://github.com/vitejs/vite/blob/af2aa09575229462635b7cbb6d248ca853057ba2/packages/vite/src/node/plugins/resolve.ts#L1056-L1080

But, from Vitest standpoint, suddenly injecting --conditions would be a breaking change, so I thought that would be a concern at least. (I don't know maybe this is a bug fix actually?)

@sheremet-va
Copy link
Member

sheremet-va commented Jan 16, 2024

Vitest already injects conditions that you pass down in resolve.conditions config. Vitest uses Vite environment only for the source code - if the package was externalized, it is consumed by the native Node.js mechanism to improve performance, it inherits resolve.conditions. It has nothing to do with threads, please, never use execArgv in your config manually.

But to improve the interoperability we should align conditions the same way they are resolved in Vite (except module).

@sheremet-va sheremet-va added bug p3-minor-bug An edge case that only affects very specific usage (priority) p2-edge-case Bug, but has workaround or limited in scope (priority) and removed pending triage p3-minor-bug An edge case that only affects very specific usage (priority) labels Jan 16, 2024
@justin-schroeder
Copy link
Author

It has nothing to do with threads, please, never use execArgv in your config manually.

Ok! I’ll take your word for it @sheremet-va Is there a recommended work around we can suggest for our users when we ship these updates (until this issue is resolved). Something like this?

export default defineConfig({
  resolve: {
    conditions: ['development'],
  },
  test: {
    // ...
  }
})

@sheremet-va
Copy link
Member

sheremet-va commented Jan 16, 2024

Something like this?

Yes. Maybe if users don't have it already, it's better to have an if check there if it's not already in a vitest.config file:

conditions: process.env.VITEST ? ['development'] : undefined

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
3 participants