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

Rework dependencies #12190

Merged
merged 4 commits into from Dec 31, 2021
Merged

Rework dependencies #12190

merged 4 commits into from Dec 31, 2021

Conversation

Semigradsky
Copy link
Contributor

@Semigradsky Semigradsky commented Dec 26, 2021

Summary

I found that some dependencies are not being used, some have incorrect types (should be in dependencies section or devDependencies).

Test plan

},
"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"
Copy link
Contributor

@mrazauskas mrazauskas Dec 26, 2021

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": "*",
Copy link
Contributor

@mrazauskas mrazauskas Dec 26, 2021

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:

test.only('does not work on Jasmine', () => {

describe(description, () => {

@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Dec 26, 2021

Codecov Report

Merging #12190 (adc075e) into main (1b97264) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
packages/jest-runtime/src/index.ts 56.60% <ø> (ø)
packages/expect/src/utils.ts 96.53% <0.00%> (+0.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b97264...adc075e. Read the comment docs.

SimenB
SimenB approved these changes Dec 31, 2021
Copy link
Collaborator

@SimenB SimenB left a comment

thank you so much for taking the time to clean this up!

@SimenB SimenB merged commit 830f953 into facebook:main Dec 31, 2021
33 checks passed
@@ -25,7 +25,6 @@
}
},
"dependencies": {
"@babel/core": "^7.1.0",
Copy link
Contributor

@merceyz merceyz Jan 5, 2022

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

Copy link
Collaborator

@SimenB SimenB Jan 5, 2022

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

Copy link
Collaborator

@SimenB SimenB Jan 5, 2022

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

Copy link
Collaborator

@SimenB SimenB Jan 5, 2022

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

Copy link
Contributor

@merceyz merceyz Jan 5, 2022

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.

"@babel/core": "^7.8.0",

Copy link
Collaborator

@SimenB SimenB Jan 5, 2022

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 👍

Copy link
Contributor

@merceyz merceyz Jan 5, 2022

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.

@github-actions
Copy link

@github-actions github-actions bot commented Feb 5, 2022

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 Feb 5, 2022
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.

None yet

6 participants