This repository has been archived by the owner on Jan 20, 2022. It is now read-only.
Allow --force to override conflicted peerOptional #228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For these dependency graphs:
# case a
root -> (x, y@1)
x -> PEEROPTIONAL(z)
z -> PEER(y@2)
# case b
root -> (x) PEEROPTIONAL(y@1)
x -> PEEROPTIONAL(y@2)
# case c
root -> (x) PEEROPTIONAL(y@1)
x -> PEER(z)
z -> PEEROPTIONAL(y@2)
The peerOptional dependency is included in the peerSet, which would
raise an ERESOLVE conflict at the peerSet generation stage, even though
the peerOptional dependencies will not actually be added to the tree.
To address this, this commit tracks the nodes which are actually
required in the peerSet generation phase, by virtue of being
non-optionally depended upon by a required node in the peerSet.
If a conflict occurs on a node which is not in the required set during
the peerSet generation phase, we ignore it in much the same way that we
would ignore peerSet errors in metadependencies or when --force is used.
Of course, if the peerOptional dependency is _actually_ required, to
avoid a conflict with an invalid resolution present in the tree already,
and there is no suitable placement for it, then ERESOLVE will still be
raised.
Fix: #223
Fix: npm/cli#2667
PR-URL: #227
Credit: @isaacs
Close: #227
Reviewed-by: @ruyadorno
ruyadorno
approved these changes
Feb 12, 2021
|
oops, it has conflicts now that you started landing the other PRs 😅 |
isaacs
force-pushed
the
isaacs/fix-peer-optional-eresolve-unforceable
branch
from
ed7d1f6
to
c37bf6c
Compare
With a dependency graph like this: ``` root -> (a, b@1) a -> PEEROPTIONAL(b@2) ``` We do not install the peerOptional dependency by default, so even though `b@2` is included in the peerSet of `a`, it is not added to the tree. Then, the `b@1` dependency is added to satisfy root's direct dependency on it, causing the `a -> b@2` edge to become invalid. We then try to resolve the `a -> b@2` edge, and find that we cannot place it anywhere, causing an `ERESOLVE` error. However, because `b@2` is no longer a part of a peerSet sourced on the `root` node, we miss the chance to detect that it should be overridden, resulting in an `ERESOLVE` failure even when `--force` is used. This commit adds the check for `this[_force]` prior to crashing with ERESOLVE, so that cases that avoid our earlier heuristics still accept the invalid resolution when `--force` is in effect. Fix: #226 Fix: npm/cli#2504 PR-URL: #228 Credit: @isaacs Close: #228 Reviewed-by: @ruyadorno
isaacs
force-pushed
the
isaacs/fix-peer-optional-eresolve-unforceable
branch
from
c37bf6c
to
1d68907
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #227, land that first.
With a dependency graph like this:
We do not install the peerOptional dependency by default, so even though
b@2is included in the peerSet ofa, it is not added to the tree.Then, the
b@1dependency is added to satisfy root's direct dependencyon it, causing the
a -> b@2edge to become invalid.We then try to resolve the
a -> b@2edge, and find that we cannotplace it anywhere, causing an
ERESOLVEerror.However, because
b@2is no longer a part of a peerSet sourced on therootnode, we miss the chance to detect that it should be overridden,resulting in an
ERESOLVEfailure even when--forceis used.This commit adds the check for
this[_force]prior to crashing withERESOLVE, so that cases that avoid our earlier heuristics still accept
the invalid resolution when
--forceis in effect.Fix: #226
Fix: npm/cli#2504