-
Notifications
You must be signed in to change notification settings - Fork 240
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
describe how npm 7 handles peer conflicts #239
Conversation
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.
It seems like it would be more ideal to, when a dep graph would have passed with --strictPeerDeps
, ensure that future installs operate with strictPeerDeps. That way, an invalid dep graph can still be installed as users migrate to npm 7, but a valid dep graph can't suddenly become invalid without failing the install.
|
||
When the `--legacy-peer-deps` flag is set, ignore `peerDependencies` | ||
entirely. This makes npm v7 behave effectively the same as npm v3 through | ||
v6. The `--legacy-peer-deps` flag is _never_ set for `npm ls`, so that npm |
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.
so npm ls --legacy-peer-deps
will have the flag as a noop, or will fail as an invalid flag?
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.
ls
passes legacyPeerDeps: false
in the arborist options regardless of what the config says.
This leaves you vulnerable to failing the install if a meta dep changes in such a way that the peer deps become invalid (but overrideably so). Strict peer deps kind of has to be either the default always, or explicitly opt in, I think. |
``` | ||
|
||
In this example, the `z@1` dependency will be used in non-strict mode if | ||
`x` is not the root project. |
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.
this sounds a little bit confusing to me, so if z@1
is picked for the current level it means that z@2
gets duped under x
? (might be worth adding that bit here if that's the case, just for the sake of clarity for future readers)
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.
Action: update to show tree in both scenarios.
the only place where its dependencies can be placed, causing the conflict. | ||
|
||
- **Strict mode**: Raise an error and halt the ideal tree building process. | ||
- **Force mode**: Warn about the conflict. Arbitrarily keep the first peer |
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.
would this "first peer dependency" happen to be the first one declared in the package.json
file? Otherwise if there's no way at all to define it, it seem to me it might be better to just raise an error here too.
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.
It would be the first one encountered in a breadth-first traversal through the dependency graph, ordered alphabetically.
As I had mentioned on the call:
|
fd1bcbe
to
7c67796
Compare
I agree, that would be nice. I'd rather keep this RFC more focused, and leave the position of warnings as a UI issue we can iterate on, since this is already quite long and dense. But I agree, printing it below any other
There's no plan to make |
Are there any tweaks from the actual implementation that landed in arborist needed to be added/changed here @isaacs or should we just land this? |
571b400
to
42ce99d
Compare
FYI: For the people looking for the RFC, it's here now. |
rfc