vitest icon indicating copy to clipboard operation
vitest copied to clipboard

fix(coverage): istanbul provider to work with JSDOM and `process.env` usage

Open AriPerkkio opened this issue 2 years ago • 10 comments

Description

  • Fixes #3860

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • [x] It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • [x] Ideally, include a test that fails without this PR but passes with it.
  • [x] Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • [x] Run the tests with pnpm test:ci.

Documentation

  • [ ] If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • [x] Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

AriPerkkio avatar Aug 06 '23 16:08 AriPerkkio

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
Latest commit fd4e7734b7ffeca1517fd688658aeac8d18c2c3c
Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/64dbb10fe0fc9900076d94f5

netlify[bot] avatar Aug 06 '23 16:08 netlify[bot]

Vite has a guide for these cases: https://vitejs.dev/guide/env-and-mode.html#production-replacement

For JavaScript strings, you can break the string up with a Unicode zero-width space, e.g. 'import.meta\u200b.env.MODE'.

Instead of replacing the process.env back and forth, should we just replace them to process\u200b.env and leave that way?

This PR is now verifying that Node specific process.env works when using web transforms. Are there any web related stuff that gets transformed when running in Node?

AriPerkkio avatar Aug 06 '23 16:08 AriPerkkio

I feel like we need a more general solution. process.env.NODE_ENV is replaced even when we don't need it to be (https://github.com/vitest-dev/vitest/issues/3933). Maybe we can hijack the internal Vite plugin to not replace it? The plugin is here: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/clientInjections.ts

sheremet-va avatar Aug 11 '23 11:08 sheremet-va

I feel like we need a more general solution. process.env.NODE_ENV is replaced even when we don't need it to be (#3933). Maybe we can hijack the internal Vite plugin to not replace it? The plugin is here: vitejs/vite@main/packages/vite/src/node/plugins/clientInjections.ts

Implemented it here: https://github.com/vitest-dev/vitest/pull/3957

sheremet-va avatar Aug 15 '23 11:08 sheremet-va

#3957 looks a lot more reliable than this one. I'll convert this into test(coverage): JSDOM + process.env usage later.

AriPerkkio avatar Aug 15 '23 13:08 AriPerkkio

Browser tests are still failing with SyntaxError: Unexpected identifier 'test'

AriPerkkio avatar Aug 15 '23 17:08 AriPerkkio

Browser tests are still failing with SyntaxError: Unexpected identifier 'test'

Interesting. Browser has a separate server so it's not affected by hijacking (and shouldn't be). process.env.NODE_ENV should be transformed there

sheremet-va avatar Aug 16 '23 19:08 sheremet-va

process.env.NODE_ENV should be transformed there

Yes, it is transformed and it breaks the inline source maps where process.env.NODE_ENV is used inside a string.

Any ideas how this should be handled in browser environment?

AriPerkkio avatar Aug 26 '23 08:08 AriPerkkio

Linked issue is closed, should we lose this one? 🤔

sheremet-va avatar Jul 01 '24 16:07 sheremet-va

Yep, let's close this old PR. No one has reported issues with JSDOM so far.

AriPerkkio avatar Jul 02 '24 05:07 AriPerkkio