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(resolve): replace read-pkg-up with escalade #10558

Merged
merged 9 commits into from
Oct 1, 2020

Conversation

TrySound
Copy link
Contributor

Summary

Ref https://packagephobia.com/result?p=read-pkg-up https://packagephobia.com/result?p=escalade

Read-pkg-up package is quite big and have a lot of dependencies.
It also handles cases not necessary for jest. Beautiful JSON parsing
errors are not relevant and can be replaced with builtin JSON.parse.

Test plan

Not sure about this. Looks like es modules support was not covered by tests.

Ref https://packagephobia.com/result?p=read-pkg-up https://packagephobia.com/result?p=escalade

Read-pkg-up package is quite big and have a lot of dependencies.
It also handles cases not necessary for jest. Beautiful JSON parsing
errors are not relevant and can be replaced with builtin JSON.parse.
@TrySound
Copy link
Contributor Author

TrySound commented Sep 27, 2020

Aha, looks like testing on 13.2 is not a good idea. Native ESM support is broken until 13.7. The issue is the same as in many packages with es modules support.

Error: No valid exports main found for '/.../jest/node_modules/yargs'

@jeysal jeysal requested a review from SimenB September 27, 2020 19:16
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Left some inline comments. In addition to those, could you add a changelog entry?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@SimenB SimenB merged commit 0a9e77d into jestjs:master Oct 1, 2020
@TrySound TrySound deleted the escalade branch October 1, 2020 13:11
@gwmccull
Copy link

gwmccull commented Oct 6, 2020

for anyone stumbling across this issue due to this error:

Error: No valid exports main found for '/.../jest/node_modules/yargs'

you need to update the version of node that you're using to >=13.7

@SimenB
Copy link
Member

SimenB commented Oct 6, 2020

Let's revert and re-land for Jest 27. At that point we'll drop support for Node 13 anyways (13 has been EOL since June).

SimenB added a commit to SimenB/jest that referenced this pull request Oct 6, 2020
@TrySound
Copy link
Contributor Author

TrySound commented Oct 6, 2020

@SimenB Are you gonna revert and escalade and yargs as well? Are you sure any more dependencies won't add it?

@SimenB
Copy link
Member

SimenB commented Oct 6, 2020

yarn why escalade only points to browserlist now, which we have as a dev dep from babel, so should be good

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants