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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions lib/can-place-dep.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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