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

refactor: remove jest condition #8783

Closed
wants to merge 1 commit into from
Closed

refactor: remove jest condition #8783

wants to merge 1 commit into from

Conversation

sxzz
Copy link
Member

@sxzz sxzz commented Jun 25, 2022

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
  • Other

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

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

@netlify
Copy link

netlify bot commented Jun 25, 2022

Deploy Preview for vite-docs-main canceled.

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

@bluwy
Copy link
Member

bluwy commented Jun 25, 2022

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

@patak-dev
Copy link
Member

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 patak-dev added p1-chore Doesn't change code behavior (priority) on hold labels Jun 25, 2022
@bluwy bluwy added this to the 5.0 milestone Dec 29, 2022
@sxzz sxzz closed this by deleting the head repository Jan 27, 2023
@sxzz sxzz reopened this Feb 4, 2023
@patak-dev
Copy link
Member

/ecosystem-ci run

@patak-dev
Copy link
Member

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.

@bluwy
Copy link
Member

bluwy commented Aug 8, 2023

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

@bluwy
Copy link
Member

bluwy commented Oct 5, 2023

I'm checking this today to see what's the reason we have the code in the first place. #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.

@sxzz sxzz closed this Oct 6, 2023
@sxzz sxzz mentioned this pull request Oct 6, 2023
9 tasks
@sxzz
Copy link
Member Author

sxzz commented Oct 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change on hold p1-chore Doesn't change code behavior (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants