-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 peer requirement and warning system #6205
Conversation
In the peer resolution code, many comments became outdated or out-of-place over many refactors and code movements. We take this opportunity to fix those and add new comments for the new logic
I'm a little worried of changes done to the peer dependency system but it looks well-researched, I don't see something obviously wrong in it, and our tests keep passing, so I think it's worth merging. Nice work! |
Will this be released soon? |
(This is out in 4.3.0) Thanks for your work on this! Would it be possible to filter out the correctly supplied peer dependencies when running I now get the following on
But Running There is no indication in this output which dependency has the unresolved requirement (again, At the end of the output there is this:
But, again, it doesn't say which package is not satisfied. Now, I can just go back to
Which is the correct one - after looking through the ~800 lines of output, that is indeed the only one that doesn't support the version of React provided by my project. There is no visual indication either, or anything I can |
We are seeing the same thing as @SimenB, but with |
## What's the problem this PR addresses? When an incompatible peer warning occurs, the warning message shown during install simply uses the first peer requester found but that may not have any relation to the unsatisfied request. See #6205 (comment) (partial fix) ## How did you fix it? Actually find the first unsatisfied requester, prioritizing root requesters (i.e. direct dependencies). If an unsatisfied root requester is found, the warning message looks similar to 4.2.2 ![image](https://github.com/yarnpkg/berry/assets/41266433/08a3b91e-ed03-4002-9f60-e982a8d7189b) ``` ➤ YN0000: ┌ Post-resolution validation ➤ YN0060: │ react is listed by your project with version 18.3.1 (pfa021), which doesn't satisfy what reflux and other dependencies request (but they have non-overlapping ranges!). ➤ YN0086: │ Some peer dependencies are incorrectly met by your project; run yarn explain peer-requirements <hash> for details, where <hash> is the six-letter p-prefixed code. ➤ YN0000: └ Completed ``` If all root requesters are satisfied, then an unsatisfied requester is found and reported with one of the corresponding root requesters ![image](https://github.com/yarnpkg/berry/assets/41266433/753f7b94-2151-47b0-ac04-69621de8e355) ``` ➤ YN0000: ┌ Post-resolution validation ➤ YN0060: │ react is listed by your project with version 18.3.1 (pfa021), which doesn't satisfy what reflux (via @local/test-reflux-dependent) and other dependencies request (but they have non-overlapping ranges!). ➤ YN0086: │ Some peer dependencies are incorrectly met by your project; run yarn explain peer-requirements <hash> for details, where <hash> is the six-letter p-prefixed code. ➤ YN0000: └ Completed ``` One concern I had was that it will make the message longer, but we are already way past the common terminal width so I don't think that's a big issue. ## Checklist <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [x] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [x] I will check that all automated PR checks pass before the PR gets reviewed. --------- Co-authored-by: Maël Nison <nison.mael@gmail.com>
What's the problem this PR addresses?
There are a number of issues with the current peer requirement and peer warning system:
Firstly, since v4, we lost the ability to list all peer requirements in the project and to explain missing peer dependency warnings, which is a huge degradation to DX.
Secondly, also since v4, transitive peer warnings are no longer displayed. The rationale given was "those are not actionable". But they are actionable because we have
packageExtensions
.More importantly, there are no indication nor discoverability when this happens. Yarn exits without even a warning when transitive peer warnings can put the project in an invalid state. Even when the user somehow knows that there are transitive peer warnings, they are impossible to investigate and fix without the proper tools.
Thirdly, as I just reported, the peer warning aggregation is aggregating unrelated peer requests, causing
yarn explain peer-requirements
to give incorrect advice. #6203Finally, the peer resolution pipeline has been refactored and added to many times over the years which make the code quite difficult to understand. This is compounded by misplaced comments and overloading terms like "peer descriptor" in the code.
Closes #5841
Closes #5977
Closes #6016
Closes #6118
Closes #6203
How did you fix it?
On the last problem, it is quite impossible to completely solve in one go, so I only took small bites and at least rewrote some of the comments to better capture the current peer pipeline. I have also unified the terminology and change variable names to use those terminology to reduce ambiguity. Maybe we should add these to the lexicon?
Let's say
A
(regular-)depends onB
, which peer-depends onC
A
also depends onC
In the scope of this single level
A@1.0.0
is known as the subject or requesteeB@1.1.0
is known as the requester. Also called the virtualized package in the code because of what the pipeline is doingC
is known as the peer ident and the descriptorC@^1.0.0
is known as the peer descriptorC@^1.2.0
is the provided descriptor. Similarly,C@1.3.0
is the provided locator/package. Provision can refer to any of those depending on context.A
's dependency.A
itself could be used if its ident were matching, or a package can be not provided at all.Based on those we can create some composite data structures:
With those concepts established, the core of this PR changes the peer requirement and peer warning system to use a tree-based structure. The nodes of the tree are either peer requests or peer requirements. A peer request's children are the requests it forwards, and a peer requirement's children are the requests it includes. Note that a tree can only ever include peer requirements and requests regarding a single peer ident.
By storing all peer requirement nodes in
Project
, other places can easily display information on peer requirements/requests/warnings by retrieving them form the tree (using the new tree-view UI, even). This allows us to reimplementyarn explain peer-requirements
andyarn explain peer-requirements <hash>
to list all requirements and explain any peer requirement (even without warnings) respectively. This also fixes #6203 because the peer requirement nodes correctly group the peer requests.To solve the discoverability problem without bringing back the clutter, I've added an additional CTA for transitive peer warnings, so all transitive peer warnings are reduced to a single line.
A user wishing to view the transitive warnings can do so with the reimplemented
yarn explain peer-requirements
command.Lastly, I don't know how much of the original peer requirement and warning system matter for BC. For now, I have recreated the data structures created by the original system, except the aggregated warnings which is the wrong abstraction as noted. I have added TODO comments to remove those code for the next major. Please advice if the original system can be safely removed in a minor.
Checklist