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

Add --strict-peer-deps option #1819

Closed
wants to merge 1 commit into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Sep 17, 2020

This is the CLI portion of npm/arborist#136

@ljharb
Copy link
Contributor

ljharb commented Sep 17, 2020

Continuing the clarification from the linked issue - by default, will npm install ever succeed where npm ls fails?

@isaacs
Copy link
Contributor Author

isaacs commented Sep 17, 2020

Yes. If you npm install it might put invalid peer dependencies in place, in cases where no non-conflicting resolution can be found, but a "least bad" resolution can be inferred from the dependency by the package bringing in the conflicted peer deps.

For example:

x -> (y@1, z@2)
y -> PEER(z@1)

If you npm install y@2 and then npm install x, it'll need to have y nested under x, and it'll only be able to get z@2 at a peer (or higher) level, due to x's dependency on it. But, we can infer that z@2 is fine, at least for the ways in which x uses y.

This is a very common situation. So far, every real-world ERESOLVE we've encountered matches this pattern. That's why we went the direction of making --force just accept the "incorrect" inferred resolution. But.... why not just make that the default, rather than making the user apply --force, which has other (possibly unwanted) effects? Why make them run the installation again?

For ls, however, the goal is different. It is there to show you exactly what's out of place or needing investigation. It doesn't exit 0 unless every edge it walks is valid. So, if you have a tree like this:

project
+-- x
|   +-- y@1
|   +-- z@2 (valid for x, not valid for y's peer dep)
+-- y@2

then it will report that the link from node_modules/x/node_modules/y to node_modules/x/node_modules/z is invalid, as its contract requires.

@ljharb
Copy link
Contributor

ljharb commented Sep 18, 2020

That seems very unfortunate. One of the best changes in npm 7 was that I'd no longer need to run npm ls in almost 300 CI jobs to ensure graph was valid, because npm install would do that for me. One of the greatest mistakes in npm < 7 was necessitating this step to ensure the graph was valid, and it is a massive loss to propagate that mistake in v7. I severely hope you reconsider.

@isaacs
Copy link
Contributor Author

isaacs commented Sep 18, 2020

@ljharb One way to get what you're looking for is to add --strict-peer-deps on your installs. In the scenario described above, that would fail the install.

I think there's a strong case to be made to make that the default in npm v8, especially if the more useful warnings help drive people towards correcting these issues. But the feedback we've been getting is that it's too disruptive right now, and this provides an on-ramp to having that functionality there in the first place.

Another approach which we could consider, but I don't love, is to let the install proceed and do its thing, but still exit with a non-zero status. The hazard there is that it's going to be just as breaking for anyone running installs in CI, and it's a bit weird to exit with an error code if we successfully did exactly what the user asked for.

@ljharb
Copy link
Contributor

ljharb commented Sep 18, 2020

I don't think it's weird. When npm ls exits nonzero, the dep graph is invalid, and nothing should be expected to work reliably.

Can you elaborate on this feedback you've been getting? Is there a public issue where this feedback has been collected? I'd love to understand if the disruption is fixable by fixing invalid peer dep ranges, or if it's a harder problem than that to solve - if the former, I think any disruption is worth it to incentivize properly specifying peer deps.

@darcyclarke darcyclarke added semver:major backwards-incompatible breaking changes Release 7.x work is associated with a specific npm 7 release labels Sep 18, 2020
Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seeing as how all this change does is allow passing the option to arborist, 👍 from me

ruyadorno pushed a commit that referenced this pull request Sep 22, 2020
This is the CLI portion of npm/arborist#136

PR-URL: #1819
Credit: @isaacs
Close: #1819
Reviewed-by: @ruyadorno
@ruyadorno ruyadorno closed this Sep 22, 2020
@darcyclarke darcyclarke added this to the OSS - Sprint 15 milestone Sep 25, 2020
@isaacs isaacs deleted the isaacs/strict-peer-deps branch October 2, 2020 21:43
kevinoid added a commit to kevinoid/node-project-template that referenced this pull request Feb 10, 2021
CI should fail if there are conflicting peerDependencies.
See npm/cli#1819 (comment)

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants