-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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(jest-resolve): disable jest-pnp-resolver
for Yarn 2
#10847
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing unusual in the code and given this comes from someone involved in Yarn and Maël has +1ed, I'll just trust the reasoning behind it :D
We also have e2e tests I think soooo...
Yay! 🎉 The PnP integration test fails on CI, tho. Also, can you remove https://github.com/facebook/jest/blob/a66eec74b55dc98ab072be330c13c54c35cba5f7/packages/jest-resolve/src/defaultResolver.ts#L26-L33? |
9570734
to
af03f02
Compare
jest-pnp-resolver
for Yarn 2
af03f02
to
f0c5178
Compare
I can't actually remove the plugin as that would be a breaking change for Yarn 1 PnP users, so instead I disabled it for Yarn 2. I updated the test and it passes locally so lets hope the CI agrees |
👍 We could consider dropping v1 support in a future major |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
In #9520
jest-resolve
was switched to useresolve
which, because Yarn 2 patches it, supports PnP. This makesjest-pnp-resolver
unnecessary but it can't be removed as that would be a breaking change for Yarn 1 PnP users but it can be disabled for Yarn 2.Main reason for doing this is that
jest-pnp-resolver
doesn't support projects where there are multiplepnpapi
instances but the patch inresolve
does.Test plan
Tests should still pass