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

Commit

Permalink
Fix additional peerOptional conflict cases
Browse files Browse the repository at this point in the history
case D

```
root -> (x, z@1)
x -> PEEROPTIONAL(y)
y -> PEER(z@2)
```

case E

```
root -> (x, z@1)
x -> PEEROPTIONAL(y) PEER(z@1)
y -> PEER(z@2)
```

case F

```
root -> (x, z@1)
x -> PEEROPTIONAL(y) PEER(z@1) PEER(w@1)
w -> PEER(z@1)
y -> PEER(z@2)
```

Case F properly reproduces the failure seen by installing
[email protected] alongside typescript@4.

Fix: #236

cc: @kyliau

PR-URL: #237
Credit: @isaacs
Close: #237
Reviewed-by: @isaacs
  • Loading branch information
isaacs committed Feb 18, 2021
1 parent b0997c9 commit f25367d
Show file tree
Hide file tree
Showing 42 changed files with 1,370 additions and 3 deletions.
7 changes: 5 additions & 2 deletions lib/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -1121,8 +1121,11 @@ This is a one-time fix-up, please be patient...
// we don't like it. always fail strictly, always allow forcibly or
// in non-strict mode if it's not our fault. don't warn here, because
// we are going to warn again when we place the deps, if we end up
// overriding for something else.
if (conflictOK)
// overriding for something else. If the thing that has this dep
// isn't also required, then there's a good chance we won't need it,
// so allow it for now and let it conflict if it turns out to actually
// be necessary for the installation.
if (conflictOK || !required.has(edge.from))
continue

// ok, it's the root, or we're in unforced strict mode, so this is bad
Expand Down
252 changes: 252 additions & 0 deletions tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13531,6 +13531,258 @@ ArboristNode {
}
`

exports[`test/arborist/build-ideal-tree.js TAP do not ERESOLVE on peerOptionals that are ignored anyway case d > must match snapshot 1`] = `
ArboristNode {
"children": Map {
"@isaacs/testing-peer-optional-conflict-d-x" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "@isaacs/testing-peer-optional-conflict-d-x",
"spec": "1",
"type": "prod",
},
},
"edgesOut": Map {
"@isaacs/testing-peer-optional-conflict-d-y" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-d-y",
"spec": "1",
"to": null,
"type": "peerOptional",
},
},
"location": "node_modules/@isaacs/testing-peer-optional-conflict-d-x",
"name": "@isaacs/testing-peer-optional-conflict-d-x",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/d/node_modules/@isaacs/testing-peer-optional-conflict-d-x",
"resolved": "https://registry.npmjs.org/@isaacs/testing-peer-optional-conflict-d-x/-/testing-peer-optional-conflict-d-x-1.0.0.tgz",
"version": "1.0.0",
},
"@isaacs/testing-peer-optional-conflict-d-z" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "@isaacs/testing-peer-optional-conflict-d-z",
"spec": "1",
"type": "prod",
},
},
"location": "node_modules/@isaacs/testing-peer-optional-conflict-d-z",
"name": "@isaacs/testing-peer-optional-conflict-d-z",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/d/node_modules/@isaacs/testing-peer-optional-conflict-d-z",
"resolved": "https://registry.npmjs.org/@isaacs/testing-peer-optional-conflict-d-z/-/testing-peer-optional-conflict-d-z-1.0.0.tgz",
"version": "1.0.0",
},
},
"edgesOut": Map {
"@isaacs/testing-peer-optional-conflict-d-x" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-d-x",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-d-x",
"type": "prod",
},
"@isaacs/testing-peer-optional-conflict-d-z" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-d-z",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-d-z",
"type": "prod",
},
},
"location": "",
"name": "d",
"packageName": "@isaacs/testing-peer-optional-conflict-d",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/d",
"version": "1.0.0",
}
`

exports[`test/arborist/build-ideal-tree.js TAP do not ERESOLVE on peerOptionals that are ignored anyway case e > must match snapshot 1`] = `
ArboristNode {
"children": Map {
"@isaacs/testing-peer-optional-conflict-e-x" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "@isaacs/testing-peer-optional-conflict-e-x",
"spec": "1",
"type": "prod",
},
},
"edgesOut": Map {
"@isaacs/testing-peer-optional-conflict-e-y" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-e-y",
"spec": "1",
"to": null,
"type": "peerOptional",
},
"@isaacs/testing-peer-optional-conflict-e-z" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-e-z",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-e-z",
"type": "peer",
},
},
"location": "node_modules/@isaacs/testing-peer-optional-conflict-e-x",
"name": "@isaacs/testing-peer-optional-conflict-e-x",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/e/node_modules/@isaacs/testing-peer-optional-conflict-e-x",
"resolved": "https://registry.npmjs.org/@isaacs/testing-peer-optional-conflict-e-x/-/testing-peer-optional-conflict-e-x-1.0.0.tgz",
"version": "1.0.0",
},
"@isaacs/testing-peer-optional-conflict-e-z" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "@isaacs/testing-peer-optional-conflict-e-z",
"spec": "1",
"type": "prod",
},
EdgeIn {
"from": "node_modules/@isaacs/testing-peer-optional-conflict-e-x",
"name": "@isaacs/testing-peer-optional-conflict-e-z",
"spec": "1",
"type": "peer",
},
},
"location": "node_modules/@isaacs/testing-peer-optional-conflict-e-z",
"name": "@isaacs/testing-peer-optional-conflict-e-z",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/e/node_modules/@isaacs/testing-peer-optional-conflict-e-z",
"resolved": "https://registry.npmjs.org/@isaacs/testing-peer-optional-conflict-e-z/-/testing-peer-optional-conflict-e-z-1.0.0.tgz",
"version": "1.0.0",
},
},
"edgesOut": Map {
"@isaacs/testing-peer-optional-conflict-e-x" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-e-x",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-e-x",
"type": "prod",
},
"@isaacs/testing-peer-optional-conflict-e-z" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-e-z",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-e-z",
"type": "prod",
},
},
"location": "",
"name": "e",
"packageName": "@isaacs/testing-peer-optional-conflict-e",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/e",
"version": "1.0.0",
}
`

exports[`test/arborist/build-ideal-tree.js TAP do not ERESOLVE on peerOptionals that are ignored anyway case f > must match snapshot 1`] = `
ArboristNode {
"children": Map {
"@isaacs/testing-peer-optional-conflict-f-w" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "node_modules/@isaacs/testing-peer-optional-conflict-f-x",
"name": "@isaacs/testing-peer-optional-conflict-f-w",
"spec": "1",
"type": "peer",
},
},
"edgesOut": Map {
"@isaacs/testing-peer-optional-conflict-f-z" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-f-z",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-f-z",
"type": "peer",
},
},
"location": "node_modules/@isaacs/testing-peer-optional-conflict-f-w",
"name": "@isaacs/testing-peer-optional-conflict-f-w",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/f/node_modules/@isaacs/testing-peer-optional-conflict-f-w",
"peer": true,
"resolved": "https://registry.npmjs.org/@isaacs/testing-peer-optional-conflict-f-w/-/testing-peer-optional-conflict-f-w-1.0.0.tgz",
"version": "1.0.0",
},
"@isaacs/testing-peer-optional-conflict-f-x" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "@isaacs/testing-peer-optional-conflict-f-x",
"spec": "1",
"type": "prod",
},
},
"edgesOut": Map {
"@isaacs/testing-peer-optional-conflict-f-w" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-f-w",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-f-w",
"type": "peer",
},
"@isaacs/testing-peer-optional-conflict-f-y" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-f-y",
"spec": "1",
"to": null,
"type": "peerOptional",
},
"@isaacs/testing-peer-optional-conflict-f-z" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-f-z",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-f-z",
"type": "peer",
},
},
"location": "node_modules/@isaacs/testing-peer-optional-conflict-f-x",
"name": "@isaacs/testing-peer-optional-conflict-f-x",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/f/node_modules/@isaacs/testing-peer-optional-conflict-f-x",
"resolved": "https://registry.npmjs.org/@isaacs/testing-peer-optional-conflict-f-x/-/testing-peer-optional-conflict-f-x-1.0.0.tgz",
"version": "1.0.0",
},
"@isaacs/testing-peer-optional-conflict-f-z" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "@isaacs/testing-peer-optional-conflict-f-z",
"spec": "1",
"type": "prod",
},
EdgeIn {
"from": "node_modules/@isaacs/testing-peer-optional-conflict-f-w",
"name": "@isaacs/testing-peer-optional-conflict-f-z",
"spec": "1",
"type": "peer",
},
EdgeIn {
"from": "node_modules/@isaacs/testing-peer-optional-conflict-f-x",
"name": "@isaacs/testing-peer-optional-conflict-f-z",
"spec": "1",
"type": "peer",
},
},
"location": "node_modules/@isaacs/testing-peer-optional-conflict-f-z",
"name": "@isaacs/testing-peer-optional-conflict-f-z",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/f/node_modules/@isaacs/testing-peer-optional-conflict-f-z",
"resolved": "https://registry.npmjs.org/@isaacs/testing-peer-optional-conflict-f-z/-/testing-peer-optional-conflict-f-z-1.0.0.tgz",
"version": "1.0.0",
},
},
"edgesOut": Map {
"@isaacs/testing-peer-optional-conflict-f-x" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-f-x",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-f-x",
"type": "prod",
},
"@isaacs/testing-peer-optional-conflict-f-z" => EdgeOut {
"name": "@isaacs/testing-peer-optional-conflict-f-z",
"spec": "1",
"to": "node_modules/@isaacs/testing-peer-optional-conflict-f-z",
"type": "prod",
},
},
"location": "",
"name": "f",
"packageName": "@isaacs/testing-peer-optional-conflict-f",
"path": "{CWD}/test/fixtures/peer-optional-eresolve/f",
"version": "1.0.0",
}
`

exports[`test/arborist/build-ideal-tree.js TAP do not add shrinkwrapped deps > expect resolving Promise 1`] = `
ArboristNode {
"children": Map {
Expand Down
2 changes: 1 addition & 1 deletion test/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -2509,7 +2509,7 @@ t.test('do not ERESOLVE on peerOptionals that are ignored anyway', t => {
// this simulates three cases where a conflict occurs during the peerSet
// generation phase, but will not manifest in the tree building phase.
const base = resolve(fixtures, 'peer-optional-eresolve')
const cases = ['a', 'b', 'c']
const cases = ['a', 'b', 'c', 'd', 'e', 'f']
t.plan(cases.length)
for (const c of cases) {
t.test(`case ${c}`, async t => {
Expand Down
17 changes: 17 additions & 0 deletions test/fixtures/peer-optional-eresolve/d/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# peer optional failure D

```
root -> (x, z@1)
x -> PEEROPTIONAL(y)
y -> PEER(z@2)
```

[npm/arborist#223](https://github.com/npm/arborist/issues/223)

[npm/arborist#236](https://github.com/npm/arborist/issues/236)

Subtly similar to case A, but note y and z swapped, so that they are
resolved in the reverse order (because deps are alphabetically sorted for
consistency). This is relevant, because it ensures that there's no dep
present in the peerSet until _after_ the conflict is encountered, and that
code path was not being hit.
8 changes: 8 additions & 0 deletions test/fixtures/peer-optional-eresolve/d/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "@isaacs/testing-peer-optional-conflict-d",
"version": "1.0.0",
"dependencies": {
"@isaacs/testing-peer-optional-conflict-d-x": "1",
"@isaacs/testing-peer-optional-conflict-d-z": "1"
}
}
12 changes: 12 additions & 0 deletions test/fixtures/peer-optional-eresolve/d/x/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"name": "@isaacs/testing-peer-optional-conflict-d-x",
"version": "1.0.0",
"peerDependencies": {
"@isaacs/testing-peer-optional-conflict-d-y": "1"
},
"peerDependenciesMeta": {
"@isaacs/testing-peer-optional-conflict-d-y": {
"optional": true
}
}
}
7 changes: 7 additions & 0 deletions test/fixtures/peer-optional-eresolve/d/y/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "@isaacs/testing-peer-optional-conflict-d-y",
"version": "1.0.0",
"peerDependencies": {
"@isaacs/testing-peer-optional-conflict-d-z": "2"
}
}
4 changes: 4 additions & 0 deletions test/fixtures/peer-optional-eresolve/d/z/1/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "@isaacs/testing-peer-optional-conflict-d-z",
"version": "1.0.0"
}
4 changes: 4 additions & 0 deletions test/fixtures/peer-optional-eresolve/d/z/2/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "@isaacs/testing-peer-optional-conflict-d-z",
"version": "2.0.0"
}
13 changes: 13 additions & 0 deletions test/fixtures/peer-optional-eresolve/e/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# peer optional failure E

```
root -> (x, z@1)
x -> PEEROPTIONAL(y) PEER(z@1)
y -> PEER(z@2)
```

[npm/arborist#223](https://github.com/npm/arborist/issues/223)

[npm/arborist#236](https://github.com/npm/arborist/issues/236)

The same as case D, but with the addition of a `x->z` peerDep.
8 changes: 8 additions & 0 deletions test/fixtures/peer-optional-eresolve/e/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "@isaacs/testing-peer-optional-conflict-e",
"version": "1.0.0",
"dependencies": {
"@isaacs/testing-peer-optional-conflict-e-x": "1",
"@isaacs/testing-peer-optional-conflict-e-z": "1"
}
}
13 changes: 13 additions & 0 deletions test/fixtures/peer-optional-eresolve/e/x/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"name": "@isaacs/testing-peer-optional-conflict-e-x",
"version": "1.0.0",
"peerDependencies": {
"@isaacs/testing-peer-optional-conflict-e-y": "1",
"@isaacs/testing-peer-optional-conflict-e-z": "1"
},
"peerDependenciesMeta": {
"@isaacs/testing-peer-optional-conflict-e-y": {
"optional": true
}
}
}
7 changes: 7 additions & 0 deletions test/fixtures/peer-optional-eresolve/e/y/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "@isaacs/testing-peer-optional-conflict-e-y",
"version": "1.0.0",
"peerDependencies": {
"@isaacs/testing-peer-optional-conflict-e-z": "2"
}
}
4 changes: 4 additions & 0 deletions test/fixtures/peer-optional-eresolve/e/z/1/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "@isaacs/testing-peer-optional-conflict-e-z",
"version": "1.0.0"
}
4 changes: 4 additions & 0 deletions test/fixtures/peer-optional-eresolve/e/z/2/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "@isaacs/testing-peer-optional-conflict-e-z",
"version": "2.0.0"
}
Loading

0 comments on commit f25367d

Please sign in to comment.