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 peer requirement and warning system #6205

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

clemyan
Copy link
Contributor

@clemyan clemyan commented Apr 4, 2024

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. #6203


Finally, 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

  • Package A (regular-)depends on B, which peer-depends on C
  • Package A also depends on C
A@1.0.0 --> B@^1.0.0 (resolves to B@1.1.0) ==> C@^1.0.0
        --> C@^1.2.0 (resolves to C@1.3.0)

In the scope of this single level

  • The package A@1.0.0 is known as the subject or requestee
  • The package B@1.1.0 is known as the requester. Also called the virtualized package in the code because of what the pipeline is doing
  • The ident C is known as the peer ident and the descriptor C@^1.0.0 is known as the peer descriptor
  • The descriptor C@^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.
    • Note that the provision is not necessarily 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:

  • A peer request encapsulates a requester + peer ident.
    • If a subject does not provide to the request itself, but instead requests a peer dependency under the same ident, that subject and the new peer request it issues are both said to forward the original peer request.
  • A peer requirement encapsulates a subject + peer ident + one or more peer requests. This captures the fact that a single subject that depends on multiple peer requesters can satisfy multiple peer requests at the same time
    • Note that peer requests and peer requirements is a many-to-many relationship. A peer request is included in multiple peer requirements if the requester is depended upon multiple times (which can happen due to deduplication).
    • A peer requirement is said to be a root peer requirement if the subject does not forward the peer requests.

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.

Untitled Diagram

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 reimplement yarn explain peer-requirements and yarn 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.

image


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.

➤ YN0000: ┌ Post-resolution validation
➤ YN0002: │ workspace-b@workspace:workspace-b doesn't provide p (p427bb), requested by y.
➤ 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.
➤ YN0086: │ Some peer dependencies are incorrectly met by dependencies; run yarn explain peer-requirements for details.
➤ YN0000: └ Completed

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

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment