Fix additional peerOptional conflict cases

lib/arborist/build-ideal-tree.js

@@ -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

tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js

@@ -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 {

test/arborist/build-ideal-tree.js

@@ -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 => {

test/fixtures/peer-optional-eresolve/d/README.md

@@ -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.

test/fixtures/peer-optional-eresolve/d/package.json

@@ -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"
+  }
+}

test/fixtures/peer-optional-eresolve/d/x/package.json

@@ -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
+    }
+  }
+}

test/fixtures/peer-optional-eresolve/d/y/package.json

@@ -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"
+  }
+}

test/fixtures/peer-optional-eresolve/d/z/1/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@isaacs/testing-peer-optional-conflict-d-z",
+  "version": "1.0.0"
+}

test/fixtures/peer-optional-eresolve/d/z/2/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@isaacs/testing-peer-optional-conflict-d-z",
+  "version": "2.0.0"
+}

test/fixtures/peer-optional-eresolve/e/README.md

@@ -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.

test/fixtures/peer-optional-eresolve/e/package.json

@@ -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"
+  }
+}

test/fixtures/peer-optional-eresolve/e/x/package.json

@@ -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
+    }
+  }
+}

test/fixtures/peer-optional-eresolve/e/y/package.json

@@ -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"
+  }
+}

test/fixtures/peer-optional-eresolve/e/z/1/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@isaacs/testing-peer-optional-conflict-e-z",
+  "version": "1.0.0"
+}

test/fixtures/peer-optional-eresolve/e/z/2/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@isaacs/testing-peer-optional-conflict-e-z",
+  "version": "2.0.0"
+}