From 0ddc72b9bf2b1329e6e75487935bf89230530ec6 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Thu, 14 Oct 2021 11:50:11 -0700 Subject: [PATCH] fix: skip dep->targetEdge conflict check when placing if there is a current 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 --- lib/can-place-dep.js | 15 ++- .../arborist/build-ideal-tree.js.test.cjs | 124 +++++++++--------- tap-snapshots/test/can-place-dep.js.test.cjs | 4 + test/can-place-dep.js | 30 +++++ 4 files changed, 107 insertions(+), 66 deletions(-) diff --git a/lib/can-place-dep.js b/lib/can-place-dep.js index 6be59093c..2278e25e3 100644 --- a/lib/can-place-dep.js +++ b/lib/can-place-dep.js @@ -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 } @@ -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 diff --git a/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs b/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs index b5363796d..6c11017a3 100644 --- a/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs +++ b/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs @@ -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 { @@ -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 { @@ -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 { @@ -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 { @@ -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 { @@ -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", diff --git a/tap-snapshots/test/can-place-dep.js.test.cjs b/tap-snapshots/test/can-place-dep.js.test.cjs index 2bca9535a..73d87f6d5 100644 --- a/tap-snapshots/test/can-place-dep.js.test.cjs +++ b/tap-snapshots/test/can-place-dep.js.test.cjs @@ -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 [] ` diff --git a/test/can-place-dep.js b/test/can-place-dep.js index 2f6bb8124..6aa602ba2 100644 --- a/test/can-place-dep.js +++ b/test/can-place-dep.js @@ -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() })