vite icon indicating copy to clipboard operation
vite copied to clipboard
verified publisher icon vitejs

refactor: remove jest condition

Open sxzz opened this issue 3 years ago • 3 comments

Description

Since Vite have migrated to Vitest #8076 and removed Jest, so some conditions can be removed.

Additional context

What is the purpose of this pull request?

  • [ ] Bug fix
  • [ ] New Feature
  • [ ] Documentation update
  • [x] Other

Before submitting the PR, please make sure you do the following

  • [x] Read the Contributing Guidelines.
  • [x] Read the Pull Request Guidelines and follow the Commit Convention.
  • [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • [x] Ideally, include relevant tests that fail without this PR but pass with it.

sxzz avatar Jun 25 '22 16:06 sxzz

Deploy Preview for vite-docs-main canceled.

Name Link
Latest commit dadd55a1952b78d01d73883b943aad2fcf690b5a
Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62b73e0a25144e0008491d57

netlify[bot] avatar Jun 25 '22 16:06 netlify[bot]

Wouldn't this break for other Vite projects using Jest? Though I'm also fine with doubling-down to avoid the ESM shenanigans.

bluwy avatar Jun 25 '22 17:06 bluwy

Yes, we left this condition to avoid breaking other projects. I think we should do this change, but maybe lets wait a bit more. The ecosystem already has a lot on its hands trying to get their projects working with Vite 3.

patak-dev avatar Jun 25 '22 20:06 patak-dev

/ecosystem-ci run

patak-dev avatar Aug 07 '23 14:08 patak-dev

I'm still unsure if we could move forward with this change in Vite 5, but then the question is when we would be able to do it. Jest will still continue to be used widely for years to come.

patak-dev avatar Aug 07 '23 14:08 patak-dev

Maybe at the same time as going ESM only? We could also do a warning for Vite 5 at the mean time.

bluwy avatar Aug 08 '23 03:08 bluwy

I'm checking this today to see what's the reason we have the code in the first place. https://github.com/vitejs/vite/pull/5197 adds it but it's not quite clear how to reproduce it today to see if it's fixed. The issue isn't that ESM doesn't work in Jest, it's that Jest is emulating ESM wrongly (iiuc), and we're working around that bug.

Given that:

  1. It's a Jest bug
  2. The bug may be already fixed today
  3. It's rare to run Vite inside Jest's runtime

I think we can take a shot in removing this Jest handling. And if we get reports of Jest incompatibility post-launch, we can reevaluate and revert if needed.

bluwy avatar Oct 05 '23 09:10 bluwy

Since my forked repo and branch were accidentally removed. Created a new PR: #14544

sxzz avatar Oct 06 '23 05:10 sxzz