-
-
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
Rework dependencies #12190
Rework dependencies #12190
Conversation
}, | ||
"devDependencies": { | ||
"@jest/test-utils": "^27.4.2", | ||
"chalk": "^4.0.0", | ||
"fast-check": "^2.0.0", | ||
"immutable": "^4.0.0-rc.12", | ||
"mlh-tsd": "^0.14.1" |
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.
Right. Currently mlh-tsd
is not necessary here.
@@ -14,14 +14,15 @@ | |||
}, | |||
"dependencies": { | |||
"@jest/types": "^27.4.2", | |||
"@types/jest": "*", |
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.
Most probably @types/jest
is included here because of test
and describe
:
Codecov Report
@@ Coverage Diff @@
## main #12190 +/- ##
=======================================
Coverage 67.52% 67.53%
=======================================
Files 328 328
Lines 17244 17244
Branches 5069 5069
=======================================
+ Hits 11644 11645 +1
+ Misses 5567 5566 -1
Partials 33 33
Continue to review full report at Codecov.
|
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.
thank you so much for taking the time to clean this up!
@@ -25,7 +25,6 @@ | |||
} | |||
}, | |||
"dependencies": { | |||
"@babel/core": "^7.1.0", |
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.
babel-jest
has a peer dependency on @babel/core
so this can't be removed
babel-jest tried to access @babel/core (a peer dependency) but it isn't provided by its ancestors; this makes the require call ambiguous and unsound.
Required package: @babel/core
Required by: babel-jest@virtual:2bc57ffd75a28b22674b53a6358defcca1d286866384fd64e16f99fc2738cc5b013508c53a59bfabe730e4daa2d273f07ae36eb99455e82b33166a59e8627945#npm:27.4.6 (via /tmp/tmp.xgTsoWLcRm/.yarn/__virtual__/babel-jest-virtual-0aeed8dc4a/0/cache/babel-jest-npm-27.4.6-73245addbc-fc839d5e87.zip/node_modules/babel-jest/build/)
Ancestor breaking the chain: jest-config@virtual:bcd3bc02ca624af302dc8d7b6ba9331b5834c819f42a96fe17b1252a465930e046a8d3b3ca14dba7c4b010d958fa2b9de851d43e995d554f9d06f1f12e8ff82f#npm:27.4.6
https://github.com/yarnpkg/berry/runs/4708870979?check_suite_focus=true#step:4:45
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.
hmm, surprised our pnp test didn't pick this up
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.
there's also no peer dependency warning within this repo for the missing peer
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.
that said, we can add it back now for now. The plan is to not ship babel-jest
by default in the future, at which point this'll be cleaner
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.
It doesn't catch it because it's declared in the devDependencies
of babel-jest
and Yarn 2.x installs devDependencies
when using yarn link
(the portal:
protocol). If the PnP test used Yarn 3.x it would have caught it.
https://github.com/facebook/jest/blob/644d2d3e53536b0d67e395c0f35f8555a67beb1e/packages/babel-jest/package.json#L31
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.
cool! We'll update to v3 once we drop node 10 👍
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.
there's also no peer dependency warning within this repo for the missing peer
Yeah, we should fix that.
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
I found that some dependencies are not being used, some have incorrect types (should be in
dependencies
section ordevDependencies
).Test plan