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

[Bug]: Explaining peer warning gives incorrect advice #6203

Closed
1 task done
clemyan opened this issue Apr 3, 2024 · 2 comments · Fixed by #6205
Closed
1 task done

[Bug]: Explaining peer warning gives incorrect advice #6203

clemyan opened this issue Apr 3, 2024 · 2 comments · Fixed by #6205
Labels
bug Something isn't working reproducible This issue can be successfully reproduced

Comments

@clemyan
Copy link
Member

clemyan commented Apr 3, 2024

Self-service

  • I'd be willing to implement a fix

Describe the bug

Sometimes, due to how they are aggregated, yarn explain peer-requirements says peer requests cannot be all satisfied when they can.

Consider this dependency tree:

A --> X ==> P@^2
  --> P@1.0.0

B --> Y ==> P@^3
  --> P@1.0.0

where A and B are workspaces , --> represents a regular dependency, and ==> represents a peer dependency.

When we install this, we get an peer warning, which, when explained, gives

$ yarn explain peer-requirements p6ab7e
We have a problem with p, which is provided with version 1.0.0.
It is needed by the following direct dependencies of workspaces in your project:

  ✘ x@npm:1.0.0 [0f866] (via ^2)
  ✘ y@npm:1.0.0 [10ea6] (via ^3)

Unfortunately, put together, we found no single range that can satisfy all those peer requirements.
Your best option may be to try to upgrade some dependencies with yarn up, or silence the warning via logFilters.

However, this advice is not correct as it is easily fixable by making a and b depend on different versions of p.


This is caused by how peer warnings are aggregated by the provided package which loses information on different packages that depend on it. Peer warnings should instead be aggregated by requestee + peer request ident which are the individual knobs that the end user can turn.

To reproduce

const fs = require(`node:fs/promises`);
const p = require(`node:path`);

async function makeWorkspace(path, manifest) {
  await fs.mkdir(path);
  await fs.writeFile(p.join(path, `package.json`), JSON.stringify(manifest));
}

await makeWorkspace(`./package-x`, {
  name: `x`,
  version: `1.0.0`,
  peerDependencies: {
    p: `^2`,
  },
});
await makeWorkspace(`./package-y`, {
  name: `y`,
  version: `1.0.0`,
  peerDependencies: {
    p: `^3`,
  },
});
await makeWorkspace(`./package-p`, {
  name: `p`,
  version: `1.0.0`,
});

await makeWorkspace(`./workspace-a`, {
  name: `a`,
  version: `1.0.0`,
  dependencies: {
    x: `1.0.0`,
    p: `1.0.0`,
  },
});
await makeWorkspace(`./workspace-b`, {
  name: `b`,
  version: `1.0.0`,
  dependencies: {
    y: `1.0.0`,
    p: `1.0.0`,
  },
});

const output = await packageJsonAndInstall({
  workspaces: [
    `workspace-*`,
    `package-*`,
  ],
});
const hash = output.match(/\(p[0-9a-f]{5}\)/)[0].slice(1, -1);

await expect(yarn('explain', 'peer-requirements', hash)).resolves.not.toMatch(/no single range that can satisfy all/);

Environment

System:
    OS: Windows 10 10.0.19045
    CPU: (8) x64 Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz
  Binaries:
    Node: 21.7.0 - D:\Users\Clement\AppData\Local\Temp\xfs-cb19a917\node.CMD
    Yarn: 4.1.1 - D:\Users\Clement\AppData\Local\Temp\xfs-cb19a917\yarn.CMD
    npm: 10.5.0 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.15.4 - C:\Program Files\nodejs\pnpm.CMD

Additional context

No response

@clemyan clemyan added the bug Something isn't working label Apr 3, 2024
@yarnbot yarnbot added the reproducible This issue can be successfully reproduced label Apr 3, 2024
@yarnbot

This comment has been minimized.

@yarnbot
Copy link
Collaborator

yarnbot commented Apr 25, 2024

This issue reproduces on master:

Error: expect(received).resolves.not.toMatch(expected)

Expected pattern: not /no single range that can satisfy all/
Received string:      "We have a problem with p, which is provided with version 0.0.0-use.local.
It is needed by the following direct dependencies of workspaces in your project:

  ✘ x@workspace:package-x [f8fd9] (via ^2)
  ✘ y@workspace:package-y [b18b8] (via ^3)

Unfortunately, put together, we found no single range that can satisfy all those peer requirements.
Your best option may be to try to upgrade some dependencies with yarn up, or silence the warning via logFilters.
"
    at Object.toMatch (/github/workspace/.yarn/cache/expect-npm-24.8.0-8c7640c562-9b3e8262b0.zip/node_modules/expect/build/index.js:202:20)
    at module.exports (evalmachine.<anonymous>:54:71)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async /github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.3-558f52b79f-69609f7d06.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:57:13
    at async executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.3-558f52b79f-69609f7d06.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:18:16)
    at async executeRepro (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.3-558f52b79f-69609f7d06.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:25:12)
    at async ExecCommand.execute (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.3-558f52b79f-69609f7d06.zip/node_modules/@arcanis/sherlock/lib/commands/exec.js:26:38)
    at async ExecCommand.validateAndExecute (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-312910c71e.zip/node_modules/clipanion/lib/advanced/Command.js:161:26)
    at async Cli.run (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-312910c71e.zip/node_modules/clipanion/lib/advanced/Cli.js:74:24)
    at async Cli.runExit (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-312910c71e.zip/node_modules/clipanion/lib/advanced/Cli.js:83:28)

arcanis added a commit that referenced this issue May 17, 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](https://github.com/yarnpkg/berry/assets/41266433/166595b0-b7be-4e8c-a589-64a7d16ac462)

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](https://github.com/yarnpkg/berry/assets/41266433/7f40f8f3-53da-48d4-bb10-38a8c23ccec4)

----

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**
<!--- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working reproducible This issue can be successfully reproduced
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants