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 handle package.json exports conditions like browser #2603

Closed
6 tasks done
segevfiner opened this issue Jan 3, 2023 · 19 comments
Closed
6 tasks done

Vitest doesn't handle package.json exports conditions like browser #2603

segevfiner opened this issue Jan 3, 2023 · 19 comments

Comments

@segevfiner
Copy link
Contributor

Describe the bug

We have a package that has a browser ESM & Node CJS & ESM builds. So exports looks something like this:

  "exports": {
    "types": "./dist/node.d.ts",
    "browser": "./dist/browser.mjs",
    "import": "./dist/node.mjs",
    "require": "./dist/node.js"
  },

Vitest selects the Node ESM build even with environment: "jsdom". Annoyingly, this can be a good thing for some packages, and bad for others in tests.

See https://jestjs.io/docs/29.0/configuration#testenvironmentoptions-object for the config and special comment syntax for this issue supported by Jest.

This might be difficult to support without loader hooks though Node does have a --conditions flag.

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-4jwegg?file=test/basic.test.ts

System Info

System:
    OS: macOS 12.6.2
    CPU: (10) arm64 Apple M1 Pro
    Memory: 123.48 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.19.0 - ~/.nvm/versions/node/v16.19.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.19.0/bin/yarn
    npm: 9.2.0 - ~/.nvm/versions/node/v16.19.0/bin/npm
  Browsers:
    Chrome: 108.0.5359.124
    Safari: 16.2

Used Package Manager

npm

Validations

@segevfiner
Copy link
Contributor Author

cc @talkor

@sheremet-va
Copy link
Member

sheremet-va commented Jan 3, 2023

Browser condition usually has ESM that Node doesn't support (like imports without .js extension or no type: "module"), so yes, Vitest doesn't add it even with jsdom enabled, because it just won't run in most cases.

You can enable browser condition (or any condition), using config:

export default {
  resolve: {
    conditions: ['browser']
  }
}

Vitest passes these conditions to Node with --conditions flag that you mentioned, and also uses this flag to resolve imports inside your source code with Vite.

@segevfiner
Copy link
Contributor Author

Is this in the docs? This whole ESM, CJS, Browser, Node stuff is truly awful...

@sheremet-va
Copy link
Member

Is this in the docs?

This is part of Vite configuration, Vitest doesn't duplicate it, but mentions it:

In addition to the following options, you can also use any configuration option from Vite. For example, define to define global variables, or resolve.alias to define aliases.

This whole ESM, CJS, Browser, Node stuff is truly awful...

It truly is

@ranile
Copy link

ranile commented May 22, 2023

I ran into this issue and created a stackblitz for repro: https://stackblitz.com/edit/vitest-dev-vitest-vhpesz that uses browser mode but doesn't load browser modules.

I put

  resolve: {
    conditions: ['browser']
  }

in my vite.config.ts but it didn't help. If someone can provide a fixed version of this stackblitz, that would be very helpful.

CC: @sheremet-va

@sheremet-va
Copy link
Member

I put

conditions are for exports field. If you rely on the main field, use resolve.mainFields

@ranile
Copy link

ranile commented May 23, 2023

I've used:

resolve: {
    conditions: ['browser'],
    mainFields: ['browser'],
  },

and had the same result. Removing main and browser from package.json doesn't help either. If you could provide a working stackblitz, I would appreciate that

@segevfiner
Copy link
Contributor Author

If I have tests using both a node environment and a jsdom environment, configuring conditions, mainFields and/or resolve.alias/test.alias becomes troublesome to do per environment to resolve such issues, any tips?

@sheremet-va
Copy link
Member

If I have tests using both a node environment and a jsdom environment, configuring conditions, mainFields and/or resolve.alias/test.alias becomes troublesome to do per environment to resolve such issues, any tips?

You can use Vitest workspace for this.

@DylanPiercey
Copy link
Contributor

FYI manually telling node to use the --conditions=browser flag (or via env NODE_OPTIONS="--conditions=brower") helped me get past this. Perhaps this is something that the envs (eg jsdom) could do automatically?

@sheremet-va
Copy link
Member

Perhaps this is something that the envs (eg jsdom) could do automatically?

Using browser condition will break most application tests. It’s usually not meant to run in Node.js.

@DylanPiercey
Copy link
Contributor

DylanPiercey commented Oct 1, 2023

@sheremet-va right, Ideally you'd be able to configure this for specific envs (eg in a workspace).

@DylanPiercey
Copy link
Contributor

Also FWIW in jest that condition is loaded by default for browser envs so I'm curious why not enable that (within the test thread)? You say it would break most tests, but again this is kind of expected since the real app running (usually via bundler, but also via jest) will load the browser export.

@sheremet-va
Copy link
Member

Because it cases a ton of problems and literally breaks your tests (lookup jest issues). It is probably one of the worst decisions jest team made. browser condition may be UMD build, it can be unprocessed ESM - it can be anything. Vitest prioritizes working code and performance over semantics.

@sheremet-va
Copy link
Member

sheremet-va commented Oct 1, 2023

You can also always pass down conditions to resolve.conditions. Vitest runs workers with —condition flag. I already mentioned it in this thread.

@DylanPiercey
Copy link
Contributor

There are a large number of isomorphic modules which expect to use different apis in a browser environment which is why every bundler supports this (and every other browser oriented test runner I've used).

Having said that, I did try the conditions field and it wasn't working but I must have messed something up or there was a cache or something. I just tried that again and it is in fact working for me 👍

@segevfiner
Copy link
Contributor Author

Honestly, this whole ESM vs CJS vs Browser vs Node.js is just a nightmare to deal with, with some packages getting it wrong, and different bundlers, runtimes, and tools, all implementing some parts, and missing others, that I way too often need workarounds to get stuff working, for example: vitejs/vite#9731, microsoft/playwright#23662.

Most of this stem from Node.js strict ESM support and the outright refusal of some tools to transpile code to work with it, without having to change it, e.g. TypeScript refusing to automatically add file extensions to imports. JSON imports requiring import assertions, but tools don't add those and some don't even support them in the code (e.g. in Vue SFC). And on and on.

And don't get me started on problem packages that require workers, WASM, non-static dynamic imports, static assets and so on, as those don't even have a proper standard on how to package them up for any bundlers, leading to often needing package specific bundler plugins, at the final built application artifact, so you need to manually copy paste those plugin definitions to every resulting final app build you have that transitively uses such a package.

@sheremet-va
Copy link
Member

sheremet-va commented Oct 2, 2023

Most of this stem from Node.js strict ESM support and the outright refusal of some tools to transpile code to work with it, without having to change it, e.g. TypeScript refusing to automatically add file extensions to imports. JSON imports requiring import assertions, but tools don't add those and some don't even support them in the code (e.g. in Vue SFC). And on and on.

Since we are using an opinionated tool (Vite) under the hood, we can make certain assumptions about the code we are running, so I am planning to explore ways to make it easier to load those dependencies in Node using VM API (it looks like the Node.js team is working on it in the latest Node versions, nodejs/node#49932 doesn't fix the issue but it's a start) - there is already an option to transform those files in --experimental-vm-threads: https://vitest.dev/config/#deps-web-transformassets The main bottleneck right now is the performance - funny enough it takes longer to resolve imports than to transform code.

Alternatively, loaders API is already available but it's too unstable (every minor Node.js release breaks it).

But even then I don't think we will allow browser condition by default. Vue tests for example don't even run because test-utils use UMD build which pollutes the global namespace instead of using ESM.

@sheremet-va sheremet-va closed this as not planned Won't fix, can't repro, duplicate, stale Oct 2, 2023
@DylanPiercey
Copy link
Contributor

All I will say is that it seems a bit strange given that the behavior of Vite itself is to add the browser condition (and browser field). Fine to be opinionated though, and at least I have a work around 👍

@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants