-
-
Notifications
You must be signed in to change notification settings - Fork 266
Relax peer dependency constraints #3881
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
Conversation
When we release a new version of a controller, our Yarn constraints enforce that we must also bump any peer dependencies that rely on that controller to match the new version. And one of our policies is that if we bump a peer dependency in a package, we must release a new major version of that package. As a result, because we make changes to controllers all the time, we also end up making major releases all the time too. This is unnecessary if most of those changes aren't actually breaking. To reduce the number of major releases, this commit relaxes constraints such that a peer dependency on a controller is only required to match the major version of its corresponding non-peer dependency (which is itself required to match the current version of the controller). For instance, if the current version of `@metamask/keyring-controller` is 12.2.0, this commit would allow a peer dependency on that controller to merely be `^12.0.0`.
|
I tried to checkout on this branch and followed these two steps:
But when running Did I miss some steps? |
|
@mikesposito Ah, I should have been more clear. If you update If you update |
|
After testing the constraints a bit more, I found some bugs. I also noticed that I left some existing constraints commented out 🤦🏻 I've fixed the constraints and ensured that none were commented out. I've also updated the Testing section in the PR description to account for all the cases. |
|
General direction seems good, but wouldn't it make sense to allow divergence within the major for some situations? With the same example, imagine if |
|
@legobeat If I understand you correctly, you're saying that the constraint that all peer dependencies for the same package be the same version range should be removed? If so, I've also made that change in this PR, so two packages can have a peer dependency on e.g. |
mikesposito
left a comment
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.
Looks good!
cryptodev-2s
left a comment
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.
LGTM!
Explanation
When we release a new version of a controller, our Yarn constraints enforce that we must also bump any peer dependencies that rely on that controller to match the new version. And one of our policies is that if we bump a peer dependency in a package, we must release a new major version of that package. As a result, because we make changes to controllers all the time, we also end up making major releases all the time too. This is unnecessary if most of those changes aren't actually breaking.
To reduce the number of major releases, this commit relaxes constraints on peer dependencies in two ways:
For instance, if the current version of
@metamask/keyring-controlleris 12.2.0, a controller would be allowed to declare a peer dependency on^12.0.0. In addition, two controllers would be allowed to use slightly different versions, as long as they were major-compatible with the package (for instance, one controller could use^12.0.0and another could use^12.1.0).References
Fixes #3880.
Testing
As there are changes to a few constraints in this PR, here's how you can test it:
Open
packages/accounts-controller/package.json.Update the peer dependency on
@metamask/keyring-controllerto^12.0.0.Run
yarn constraints. It should show no output (meaning that all constraints passed).@metamask/keyring-controller(12.2.0).Update the peer dependency on
@metamask/keyring-controllerto^12.1.2.Run
yarn constraints. It should show no output (meaning that all constraints passed).@metamask/keyring-controller(12.2.0).Update the peer dependency on
@metamask/keyring-controllerto^12.1.0.Run
yarn constraints. It should show no output (meaning that all constraints passed).@metamask/keyring-controller(12.2.0).Update the peer dependency on
@metamask/keyring-controllerto^12.3.0.Run
yarn constraints. It should show the following error:@metamask/keyring-controlleris greater than the current version of@metamask/keyring-controller(12.2.0), and so the recommendation is to use this version.Update the peer dependency on
@metamask/keyring-controllerto^13.0.0.Run
yarn constraints. It should show the following error:@metamask/keyring-controlleris greater than the current version of@metamask/keyring-controller(12.2.0), and so the recommendation is to use this version.Update the peer dependency on
@metamask/keyring-controllerto^11.0.0.Run
yarn constraints. It should show the following error:@metamask/keyring-controlleris not major-compatible with the current version of@metamask/keyring-controller(12.2.0), and so the recommendation is to use this version.Remove the peer dependency on
@metamask/keyring-controller.Run
yarn constraints. It should show no output (meaning that all constraints passed).@metamask/keyring-controlleris a dev dependency in this case, and so there is no requirement that it also be a peer dependency.Move the dev dependency on
@metamask/keyring-controllertodependencies, and keep the peer dependency missing.Run
yarn constraints. It should show the following error:@metamask/keyring-controlleris required to be a peer dependency if it is a dependency.Changelog
(N/A)
Checklist