Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

fix: skip dep->targetEdge conflict check when placing if there is a current node #337

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Oct 14, 2021

Fix: npm/cli#3881

When placing a peer set containing a dependency that could not be placed
higher in the tree due to a conflict, the entire peer set would get
nested even though the current node would satisfy the condition at the
target.

For example:

root -> (k, y@1)
k -> (x)
x -> PEER(y@1||2)

When placing x it would previously result in a tree with x nested
under k even though it can be properly hoisted to the root:

root
+-- k@1
|   +-- x@1
+-- y@1

After this patch the tree will now result in x being properly hoisted:

root
+-- k@1
+-- x@1
+-- y@1

Co-authored-by: @isaacs

…urrent node

Fix: npm/cli#3881

When placing a peer set containing a dependency that could not be placed
higher in the tree due to a conflict, the entire peer set would get
nested even though the current node would satisfy the condition at the
target.

For example:

```
root -> (k, y@1)
k -> (x)
x -> PEER(y@1||2)
```

When placing `x` it would previously result in a tree with `x` nested
under `k` even though it can be properly hoisted to the root:

```
root
+-- k@1
|   +-- x@1
+-- y@1
```

After this patch the tree will now result in `x` being properly hoisted:

```
root
+-- k@1
+-- x@1
+-- y@1
```

Co-authored-by: @isaacs
Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

The only thing i would add to the commit message is saying that the reason it was being nested was because the peerSet includes a newer version that cannot be placed in the root, but which would not be placed at all anyway.

if the root could accept y@2, but just happened to have y@1, then it would have ended up being REPLACE instead of KEEP

But either one is not CONFLICT, so in either case, the right thing to do is not short-circuit, but instead continue on and see what we ought to do with the current node.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants