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

Commit

Permalink
fix: skip dep->targetEdge conflict check when placing if there is a c…
Browse files Browse the repository at this point in the history 
…urrent node (#337)

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
  • Loading branch information
@lukekarrys
lukekarrys authored Oct 15, 2021
1 parent 75a0347 commit 5fab942
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 66 deletions.
15 changes: 10 additions & 5 deletions lib/can-place-dep.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,12 @@ class CanPlaceDep {
return CONFLICT
}

if (targetEdge && !dep.satisfies(targetEdge) && targetEdge !== this.edge) {
// skip this test if there's a current node, because we might be able
// to dedupe against it anyway
if (!current &&
targetEdge &&
!dep.satisfies(targetEdge) &&
targetEdge !== this.edge) {
return CONFLICT
}

Expand All @@ -167,10 +172,10 @@ class CanPlaceDep {
const { version: newVer } = dep
const tryReplace = curVer && newVer && semver.gte(newVer, curVer)
if (tryReplace && dep.canReplace(current)) {
/* XXX-istanbul ignore else - It's extremely rare that a replaceable
* node would be a conflict, if the current one wasn't a conflict,
* but it is theoretically possible if peer deps are pinned. In
* that case we treat it like any other conflict, and keep trying */
// It's extremely rare that a replaceable node would be a conflict, if
// the current one wasn't a conflict, but it is theoretically possible
// if peer deps are pinned. In that case we treat it like any other
// conflict, and keep trying.
const cpp = this.canPlacePeers(REPLACE)
if (cpp !== CONFLICT) {
return cpp
Expand Down
124 changes: 63 additions & 61 deletions tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56865,21 +56865,21 @@ ArboristNode {
"type": "peer",
},
EdgeIn {
"from": "node_modules/standard/node_modules/eslint-plugin-import",
"from": "node_modules/standard/node_modules/eslint-plugin-es",
"name": "eslint",
"spec": "2.x - 6.x",
"spec": ">=4.19.1",
"type": "peer",
},
EdgeIn {
"from": "node_modules/standard/node_modules/eslint-plugin-node",
"from": "node_modules/standard/node_modules/eslint-plugin-import",
"name": "eslint",
"spec": ">=5.16.0",
"spec": "2.x - 6.x",
"type": "peer",
},
EdgeIn {
"from": "node_modules/standard/node_modules/eslint-plugin-node/node_modules/eslint-plugin-es",
"from": "node_modules/standard/node_modules/eslint-plugin-node",
"name": "eslint",
"spec": ">=4.19.1",
"spec": ">=5.16.0",
"type": "peer",
},
EdgeIn {
Expand Down Expand Up @@ -57197,6 +57197,60 @@ ArboristNode {
"resolved": "https://registry.npmjs.org/eslint-config-standard-jsx/-/eslint-config-standard-jsx-8.1.0.tgz",
"version": "8.1.0",
},
"eslint-plugin-es" => ArboristNode {
"children": Map {
"regexpp" => ArboristNode {
"dev": true,
"edgesIn": Set {
EdgeIn {
"from": "node_modules/standard/node_modules/eslint-plugin-es",
"name": "regexpp",
"spec": "^3.0.0",
"type": "prod",
},
},
"location": "node_modules/standard/node_modules/eslint-plugin-es/node_modules/regexpp",
"name": "regexpp",
"path": "{CWD}/test/fixtures/yargs/node_modules/standard/node_modules/eslint-plugin-es/node_modules/regexpp",
"resolved": "https://registry.npmjs.org/regexpp/-/regexpp-3.1.0.tgz",
"version": "3.1.0",
},
},
"dev": true,
"edgesIn": Set {
EdgeIn {
"from": "node_modules/standard/node_modules/eslint-plugin-node",
"name": "eslint-plugin-es",
"spec": "^2.0.0",
"type": "prod",
},
},
"edgesOut": Map {
"eslint" => EdgeOut {
"name": "eslint",
"spec": ">=4.19.1",
"to": "node_modules/standard/node_modules/eslint",
"type": "peer",
},
"eslint-utils" => EdgeOut {
"name": "eslint-utils",
"spec": "^1.4.2",
"to": "node_modules/standard/node_modules/eslint-utils",
"type": "prod",
},
"regexpp" => EdgeOut {
"name": "regexpp",
"spec": "^3.0.0",
"to": "node_modules/standard/node_modules/eslint-plugin-es/node_modules/regexpp",
"type": "prod",
},
},
"location": "node_modules/standard/node_modules/eslint-plugin-es",
"name": "eslint-plugin-es",
"path": "{CWD}/test/fixtures/yargs/node_modules/standard/node_modules/eslint-plugin-es",
"resolved": "https://registry.npmjs.org/eslint-plugin-es/-/eslint-plugin-es-2.0.0.tgz",
"version": "2.0.0",
},
"eslint-plugin-import" => ArboristNode {
"children": Map {
"debug" => ArboristNode {
Expand Down Expand Up @@ -57351,42 +57405,6 @@ ArboristNode {
},
"eslint-plugin-node" => ArboristNode {
"children": Map {
"eslint-plugin-es" => ArboristNode {
"dev": true,
"edgesIn": Set {
EdgeIn {
"from": "node_modules/standard/node_modules/eslint-plugin-node",
"name": "eslint-plugin-es",
"spec": "^2.0.0",
"type": "prod",
},
},
"edgesOut": Map {
"eslint" => EdgeOut {
"name": "eslint",
"spec": ">=4.19.1",
"to": "node_modules/standard/node_modules/eslint",
"type": "peer",
},
"eslint-utils" => EdgeOut {
"name": "eslint-utils",
"spec": "^1.4.2",
"to": "node_modules/standard/node_modules/eslint-utils",
"type": "prod",
},
"regexpp" => EdgeOut {
"name": "regexpp",
"spec": "^3.0.0",
"to": "node_modules/standard/node_modules/eslint-plugin-node/node_modules/regexpp",
"type": "prod",
},
},
"location": "node_modules/standard/node_modules/eslint-plugin-node/node_modules/eslint-plugin-es",
"name": "eslint-plugin-es",
"path": "{CWD}/test/fixtures/yargs/node_modules/standard/node_modules/eslint-plugin-node/node_modules/eslint-plugin-es",
"resolved": "https://registry.npmjs.org/eslint-plugin-es/-/eslint-plugin-es-2.0.0.tgz",
"version": "2.0.0",
},
"ignore" => ArboristNode {
"dev": true,
"edgesIn": Set {
Expand All @@ -57403,22 +57421,6 @@ ArboristNode {
"resolved": "https://registry.npmjs.org/ignore/-/ignore-5.1.8.tgz",
"version": "5.1.8",
},
"regexpp" => ArboristNode {
"dev": true,
"edgesIn": Set {
EdgeIn {
"from": "node_modules/standard/node_modules/eslint-plugin-node/node_modules/eslint-plugin-es",
"name": "regexpp",
"spec": "^3.0.0",
"type": "prod",
},
},
"location": "node_modules/standard/node_modules/eslint-plugin-node/node_modules/regexpp",
"name": "regexpp",
"path": "{CWD}/test/fixtures/yargs/node_modules/standard/node_modules/eslint-plugin-node/node_modules/regexpp",
"resolved": "https://registry.npmjs.org/regexpp/-/regexpp-3.1.0.tgz",
"version": "3.1.0",
},
"semver" => ArboristNode {
"dev": true,
"edgesIn": Set {
Expand Down Expand Up @@ -57461,7 +57463,7 @@ ArboristNode {
"eslint-plugin-es" => EdgeOut {
"name": "eslint-plugin-es",
"spec": "^2.0.0",
"to": "node_modules/standard/node_modules/eslint-plugin-node/node_modules/eslint-plugin-es",
"to": "node_modules/standard/node_modules/eslint-plugin-es",
"type": "prod",
},
"eslint-utils" => EdgeOut {
Expand Down Expand Up @@ -57621,13 +57623,13 @@ ArboristNode {
"type": "prod",
},
EdgeIn {
"from": "node_modules/standard/node_modules/eslint-plugin-node",
"from": "node_modules/standard/node_modules/eslint-plugin-es",
"name": "eslint-utils",
"spec": "^1.4.2",
"type": "prod",
},
EdgeIn {
"from": "node_modules/standard/node_modules/eslint-plugin-node/node_modules/eslint-plugin-es",
"from": "node_modules/standard/node_modules/eslint-plugin-node",
"name": "eslint-utils",
"spec": "^1.4.2",
"type": "prod",
Expand Down
4 changes: 4 additions & 0 deletions tap-snapshots/test/can-place-dep.js.test.cjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ exports[`test/can-place-dep.js TAP basic placement check tests basic placement w
Array []
`

exports[`test/can-place-dep.js TAP basic placement check tests can dedupe, cannot place peer > conflict children 1`] = `
Array []
`

exports[`test/can-place-dep.js TAP basic placement check tests cannot place peer inside of dependent > conflict children 1`] = `
Array []
`
Expand Down
30 changes: 30 additions & 0 deletions test/can-place-dep.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1356,6 +1356,36 @@ t.test('basic placement check tests', t => {
expect: OK,
})

// root -> (k, y@1)
// k -> (x)
// x -> PEER(y@1||2)
//
// root
// +-- y@1
// +-- k@1
//
// place x in root with y@2 in peerset
// https://github.com/npm/cli/issues/3881
runTest('can dedupe, cannot place peer', {
tree: new Node({
path,
pkg: { dependencies: { k: '1', y: '1' }},
children: [
{ pkg: { name: 'y', version: '1.0.0' }},
{ pkg: { name: 'k', version: '1.0.0', dependencies: { x: '' }}},
],
}),
dep: new Node({
pkg: { name: 'x', version: '1.0.0', peerDependencies: { y: '1||2' }},
}),
peerSet: [
{ pkg: { name: 'y', version: '2.0.0' }},
],
targetLoc: '',
nodeLoc: 'node_modules/k',
expect: OK,
})

t.end()
})

Expand Down

0 comments on commit 5fab942

Please sign in to comment.