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

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AriPerkkio
Copy link
Member

Description

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

  • 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.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • 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

  • 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:.

@stackblitz
Copy link

stackblitz bot commented Aug 6, 2023

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

@netlify
Copy link

netlify bot commented Aug 6, 2023

Deploy Preview for fastidious-cascaron-4ded94 canceled.

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

@AriPerkkio
Copy link
Member Author

AriPerkkio commented Aug 6, 2023

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 AriPerkkio marked this pull request as ready for review August 6, 2023 16:30
@sheremet-va
Copy link
Member

sheremet-va commented Aug 11, 2023

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: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/clientInjections.ts

@sheremet-va
Copy link
Member

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: #3957

@AriPerkkio
Copy link
Member Author

AriPerkkio commented Aug 15, 2023

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

@AriPerkkio AriPerkkio marked this pull request as draft August 15, 2023 13:54
@AriPerkkio
Copy link
Member Author

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

@sheremet-va
Copy link
Member

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

@AriPerkkio
Copy link
Member Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when using process.env.NODE_ENV with jsdom and @vitest/coverage-istanbul
2 participants