-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
chore: apply single version policy on jest #16950
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ba3f4c1:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 01dd9726d4c6e3e19fc2891275b593cc9a116d44 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
LGTM so far. I was looking at the docs for
then under
EDIT to add: I could do this in a separate PR if you prefer. |
eslint thing implemented in #16975 |
2716789
to
ba3f4c1
Compare
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.
Thanks for doing this! Approved with one super minor suggestion. (In this case it's definitely fine to admin merge rather than chasing down all the package owners to approve.)
@@ -6025,6 +6025,11 @@ [email protected]: | |||
resolved "https://registry.yarnpkg.com/axe-core/-/axe-core-3.5.0.tgz#161b17fc6ce07a5470ff179e0703957c422b2220" | |||
integrity sha512-wJqOIreoFiGjvZ1UZvGLAUs8H3QQ3cS833+6ctFcCdr/xFd5oB66mmwGWnwQlBXFFaefRt+KR+m2dY9em2RxVg== | |||
|
|||
axe-core@^3.0.3: |
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.
Probably not important in this case, but it looks like the addition of this extra axe-core version is unnecessary (maybe a leftover from some previous version of the change). So you could revert this diff and the one above.
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.
this was generated by yarn
any manual change will make all contributors quite a hard time as after running yarn
they would have modified changes.
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.
any manual change will make all contributors quite a hard time as after running
yarn
they would have modified changes.
It won't as long as:
- the combined version spec satisfies everything that's being requested (it would in this case--the full set of requested versions is the same before and after, so this actually shouldn't have changed at all; it probably only did as a leftover from some other intermediate changes)
- you run
yarn
yourself after any manual version tweaks, and include any further changes in the commit
This is something I've ended up doing a good bit of manually while we're still working on improving tooling in the area. It's more impactful when the initial duplicate is a package like @babel/core
which has a whole big tree of dependencies.
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.
One minor suggestion but otherwise LGTM, thanks!
You may also need to check again in a week or so to ensure that no new packages (created before this PR or from outdated branches) added references to any of these deps. Or possibly add that as a check in lint-staged or elsewhere--not required now but an interesting thing to think about for the future, how to enforce the policy that certain deps are specified only at the root.
Pull request checklist
$ yarn change
Description of changes
Focus areas to test