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

Commit

Permalink
Allow --force to override conflicted peerOptional
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@isaacs
isaacs committed Feb 12, 2021
1 parent 828990c commit 1d68907
Show file tree
Hide file tree
Showing 10 changed files with 1,092 additions and 7 deletions.
29 changes: 22 additions & 7 deletions lib/arborist/build-ideal-tree.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,7 @@ This is a one-time fix-up, please be patient...
// a virtual root of whatever brought in THIS node.
// so we VR the node itself if the edge is not a peer
const source = edge.peer ? peerSource : node

const virtualRoot = this[_virtualRoot](source, true)
// reuse virtual root if we already have one, but don't
// try to do the override ahead of time, since we MAY be able
Expand All @@ -827,8 +828,7 @@ This is a one-time fix-up, please be patient...
// +-- z@1
// But if x and y are loaded in the same virtual root, then they will
// be forced to agree on a version of z.
const required = edge.type === 'peerOptional' ? new Set()
: new Set([edge.from])
const required = new Set([edge.from])
const parent = edge.peer ? virtualRoot : null
const dep = vrDep && vrDep.satisfies(edge) ? vrDep
: await this[_nodeFromEdge](edge, parent, null, required)
Expand Down Expand Up @@ -1218,8 +1218,25 @@ This is a one-time fix-up, please be patient...
break
}

if (!target)
this[_failPeerConflict](edge)
// if we can't find a target, that means that the last placed checked
// (and all the places before it) had a copy already. if we're in
// --force mode, then the user has explicitly said that they're ok
// with conflicts. This can only occur in --force mode in the case
// when a node was added to the tree with a peerOptional dep that we
// ignored, and then later, that edge became invalid, and we fail to
// resolve it. We will warn about it in a moment.
if (!target) {
if (this[_force]) {
// we know that there is a dep (not the root) which is the target
// of this edge, or else it wouldn't have been a conflict.
target = edge.to.resolveParent
canPlace = KEEP
} else
this[_failPeerConflict](edge)
} else {
// it worked, so we clearly have no peer conflicts at this point.
this[_peerConflict] = null
}

this.log.silly(
'placeDep',
Expand All @@ -1230,9 +1247,6 @@ This is a one-time fix-up, please be patient...
`want: ${edge.spec || '*'}`
)

// it worked, so we clearly have no peer conflicts at this point.
this[_peerConflict] = null

// Can only get KEEP here if the original edge was valid,
// and we're checking for an update but it's already up to date.
if (canPlace === KEEP) {
Expand Down Expand Up @@ -1418,6 +1432,7 @@ This is a one-time fix-up, please be patient...
})
const entryEdge = peerEntryEdge || edge
const source = this[_peerSetSource].get(dep)

isSource = isSource || target === source
// if we're overriding the source, then we care if the *target* is
// ours, even if it wasn't actually the original source, since we
Expand Down
736 changes: 736 additions & 0 deletions tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js
View file

Large diffs are not rendered by default.

105 changes: 105 additions & 0 deletions test/arborist/build-ideal-tree.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1571,6 +1571,7 @@ t.test('more peer dep conflicts', t => {
},
error: true,
},

'prod dep directly on conflicted peer, older': {
pkg: {
dependencies: {
Expand All @@ -1580,6 +1581,7 @@ t.test('more peer dep conflicts', t => {
},
error: true,
},

'prod dep directly on conflicted peer, full peer set, newer': {
pkg: {
dependencies: {
Expand All @@ -1592,6 +1594,7 @@ t.test('more peer dep conflicts', t => {
},
error: true,
},

'prod dep directly on conflicted peer, full peer set, older': {
pkg: {
dependencies: {
Expand All @@ -1604,6 +1607,7 @@ t.test('more peer dep conflicts', t => {
},
error: true,
},

'prod dep directly on conflicted peer, meta peer set, older': {
pkg: {
dependencies: {
Expand All @@ -1615,6 +1619,7 @@ t.test('more peer dep conflicts', t => {
},
error: true,
},

'dep indirectly on conflicted peer': {
pkg: {
dependencies: {
Expand All @@ -1624,6 +1629,7 @@ t.test('more peer dep conflicts', t => {
},
error: true,
},

'collision forcing duplication, order 1': {
pkg: {
dependencies: {
Expand All @@ -1634,6 +1640,7 @@ t.test('more peer dep conflicts', t => {
error: false,
resolvable: true,
},

'collision forcing duplication, order 2': {
pkg: {
dependencies: {
Expand All @@ -1644,6 +1651,7 @@ t.test('more peer dep conflicts', t => {
error: false,
resolvable: true,
},

'collision forcing duplication via add, order 1': {
pkg: {
dependencies: {
Expand All @@ -1654,6 +1662,7 @@ t.test('more peer dep conflicts', t => {
error: false,
resolvable: true,
},

'collision forcing duplication via add, order 2': {
pkg: {
dependencies: {
Expand All @@ -1664,6 +1673,7 @@ t.test('more peer dep conflicts', t => {
error: false,
resolvable: true,
},

'collision forcing metadep duplication, order 1': {
pkg: {
dependencies: {
Expand All @@ -1674,6 +1684,7 @@ t.test('more peer dep conflicts', t => {
error: false,
resolvable: true,
},

'collision forcing metadep duplication, order 2': {
pkg: {
dependencies: {
Expand All @@ -1684,6 +1695,7 @@ t.test('more peer dep conflicts', t => {
error: false,
resolvable: true,
},

'direct collision forcing metadep duplication, order 1': {
pkg: {
dependencies: {
Expand All @@ -1694,6 +1706,7 @@ t.test('more peer dep conflicts', t => {
error: false,
resolvable: true,
},

'direct collision forcing metadep duplication, order 2': {
pkg: {
dependencies: {
Expand All @@ -1704,6 +1717,7 @@ t.test('more peer dep conflicts', t => {
error: false,
resolvable: true,
},

'dep with conflicting peers': {
pkg: {
dependencies: {
Expand All @@ -1714,6 +1728,7 @@ t.test('more peer dep conflicts', t => {
// but it is a conflict in a peerSet that the root is sourcing.
error: true,
},

'metadeps with conflicting peers': {
pkg: {
dependencies: {
Expand All @@ -1722,6 +1737,7 @@ t.test('more peer dep conflicts', t => {
},
error: false,
},

'metadep conflict that warns because source is target': {
pkg: {
dependencies: {
Expand All @@ -1732,6 +1748,7 @@ t.test('more peer dep conflicts', t => {
error: false,
resolvable: false,
},

'metadep conflict triggering the peerConflict code path': {
pkg: {
dependencies: {
Expand All @@ -1743,6 +1760,7 @@ t.test('more peer dep conflicts', t => {
resolvable: false,
},
})

t.jobs = cases.length
t.plan(cases.length)

Expand Down Expand Up @@ -2494,3 +2512,90 @@ t.test('do not ERESOLVE on peerOptionals that are ignored anyway', t => {
})
}
})

t.test('allow ERESOLVE to be forced when not in the source', async t => {
const types = [
'peerDependencies',
'optionalDependencies',
'devDependencies',
'dependencies',
]

// in these tests, the deps are both of the same type. b has a peerOptional
// dep on peer, and peer is a direct dependency of the root.
t.test('both direct and peer of the same type', t => {
t.plan(types.length)
const pj = type => ({
name: '@isaacs/conflicted-peer-optional-from-dev-dep',
version: '1.2.3',
[type]: {
'@isaacs/conflicted-peer-optional-from-dev-dep-peer': '1',
'@isaacs/conflicted-peer-optional-from-dev-dep-b': '',
},
})

for (const type of types) {
t.test(type, async t => {
const path = t.testdir({
'package.json': JSON.stringify(pj(type)),
})
t.matchSnapshot(await printIdeal(path, { force: true }), 'use the force')
t.rejects(printIdeal(path), { code: 'ERESOLVE' }, 'no force')
})
}
})

// in these, the peer is a peer dep of the root, and b is a different type
t.test('peer is peer, b is some other type', t => {
t.plan(types.length - 1)
const pj = type => ({
name: '@isaacs/conflicted-peer-optional-from-dev-dep',
version: '1.2.3',
peerDependencies: {
'@isaacs/conflicted-peer-optional-from-dev-dep-b': '',
},
[type]: {
'@isaacs/conflicted-peer-optional-from-dev-dep-peer': '1',
},
})
for (const type of types) {
if (type === 'peerDependencies')
continue
t.test(type, async t => {
const path = t.testdir({
'package.json': JSON.stringify(pj(type)),
})
t.matchSnapshot(await printIdeal(path, { force: true }), 'use the force')
t.rejects(printIdeal(path), { code: 'ERESOLVE' }, 'no force')
})
}
})

// in these, b is a peer dep, and peer is some other type
t.test('peer is peer, b is some other type', t => {
t.plan(types.length - 1)
const pj = type => ({
name: '@isaacs/conflicted-peer-optional-from-dev-dep',
version: '1.2.3',
peerDependencies: {
'@isaacs/conflicted-peer-optional-from-dev-dep-peer': '1',
},
[type]: {
'@isaacs/conflicted-peer-optional-from-dev-dep-b': '',
},
})
for (const type of types) {
if (type === 'peerDependencies')
continue
t.test(type, async t => {
const path = t.testdir({
'package.json': JSON.stringify(pj(type)),
})
t.matchSnapshot(await printIdeal(path, { force: true }), 'use the force')
t.rejects(printIdeal(path), { code: 'ERESOLVE' }, 'no force')
})
}
})

t.end()
})
12 changes: 12 additions & 0 deletions test/fixtures/conflicted-peer-optional-from-dev-dep/b/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"name": "@isaacs/conflicted-peer-optional-from-dev-dep-b",
"version": "1.0.0",
"peerDependencies": {
"@isaacs/conflicted-peer-optional-from-dev-dep-peer": "2"
},
"peerDependenciesMeta": {
"@isaacs/conflicted-peer-optional-from-dev-dep-peer": {
"optional": true
}
}
}
4 changes: 4 additions & 0 deletions test/fixtures/conflicted-peer-optional-from-dev-dep/peer/1/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "@isaacs/conflicted-peer-optional-from-dev-dep-peer",
"version": "1.0.0"
}
4 changes: 4 additions & 0 deletions test/fixtures/conflicted-peer-optional-from-dev-dep/peer/2/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "@isaacs/conflicted-peer-optional-from-dev-dep-peer",
"version": "2.0.0"
}
61 changes: 61 additions & 0 deletions test/fixtures/registry-mocks/content/isaacs/conflicted-peer-optional-from-dev-dep-b.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
{
"_id": "@isaacs/conflicted-peer-optional-from-dev-dep-b",
"name": "@isaacs/conflicted-peer-optional-from-dev-dep-b",
"dist-tags": {
"latest": "1.0.0"
},
"versions": {
"1.0.0": {
"name": "@isaacs/conflicted-peer-optional-from-dev-dep-b",
"version": "1.0.0",
"peerDependencies": {
"@isaacs/conflicted-peer-optional-from-dev-dep-peer": "2"
},
"peerDependenciesMeta": {
"@isaacs/conflicted-peer-optional-from-dev-dep-peer": {
"optional": true
}
},
"_id": "@isaacs/conflicted-peer-optional-from-dev-dep-b@1.0.0",
"_nodeVersion": "15.3.0",
"_npmVersion": "7.5.3",
"dist": {
"integrity": "sha512-2rStqBkn1QSXBjweS4lHUPH9jt4ToFqkw2nUbWA4fZBxOaYS1nkS/ST70xRlQxRW+3UdTFGc+5xk187sHlMT/A==",
"shasum": "63f188dba7ec8a3e23b09540230ba8014ee760d2",
"tarball": "https://registry.npmjs.org/@isaacs/conflicted-peer-optional-from-dev-dep-b/-/conflicted-peer-optional-from-dev-dep-b-1.0.0.tgz",
"fileCount": 1,
"unpackedSize": 299,
"npm-signature": "-----BEGIN PGP SIGNATURE-----\r\nVersion: OpenPGP.js v3.0.13\r\nComment: https://openpgpjs.org\r\n\r\nwsFcBAEBCAAQBQJgJI1ACRA9TVsSAnZWagAAbrMP+QHkDuGH+7Upc7vgCNGn\nh/BJ9oweduRP3C8DW4xviYVqUVYJdRtfN9NjA8DOA6RroecCaRONhiMPybz3\nD53u7QfKMAowj9z5KckriR8/RJ6v9EbFZWA3re/c3O6lNR8sZ0fPlw3VeOgq\nxuofI21+n3SQNDlD2N/fK8YRGkqgx4QI90IF0gb5q1k56cFP7DAqAHnRaxk9\nZ60SA8iv/lYu60+bGrnozv4H1qn6VM9m2hm/284H+HkGMOCBXSLpPLu6N1oJ\nNBy0rrp+5nhHHizWxEtTeI7xMds59p9IjMXMHSyVjnKakPYqLXeyda/dceCM\naqx5KFVyiJV+fcnqJgjgjbOLnE4+Xaz3PcSrYOfThSPgAvQBeOWk9vh0Wmuf\nMHpoONZt7NSI3ZBEbJuELpaFRO94bqaZgLqJIxDd8CiYmnbBQnOTWdzWFmCU\nQxFkwkOdQFE2oGkRRocRuWK2oD+aNegW48/tR2ckIp8apKOz9xGyL7suWmdO\n6RjCoq+9LFQJLb4q3J+ebRKD4GP0vXWHbNr/pEEPdgJI3X3q0s3jn5ig2gvU\nLj/oEkXoiPgeOvqX2n53tRtWen8Zw/hRtk06tMBUPzdg5LbGON24F7aklIrU\nCdMZwtbk/LwKXU+8bJlhTl63YB8+qoYKC7WnCqxcTvFyxZobT7FPTnGSO0wE\nHGT/\r\n=wnoR\r\n-----END PGP SIGNATURE-----\r\n"
},
"_npmUser": {
"name": "isaacs",
"email": "i@izs.me"
},
"directories": {},
"maintainers": [
{
"name": "isaacs",
"email": "i@izs.me"
}
],
"_npmOperationalInternal": {
"host": "s3://npm-registry-packages",
"tmp": "tmp/conflicted-peer-optional-from-dev-dep-b_1.0.0_1613008191679_0.24514239505031288"
},
"_hasShrinkwrap": false
}
},
"time": {
"created": "2021-02-11T01:49:51.620Z",
"1.0.0": "2021-02-11T01:49:51.832Z",
"modified": "2021-02-11T01:49:54.757Z"
},
"maintainers": [
{
"name": "isaacs",
"email": "i@izs.me"
}
],
"readme": "ERROR: No README data found!",
"readmeFilename": ""
}
Loading

0 comments on commit 1d68907

Please sign in to comment.