Do not ERESOLVE on peerOptionals not added to tree

For these dependency graphs:

    # case a
    root -> (x, y@1)
    x -> PEEROPTIONAL(z)
    z -> PEER(y@2)

    # case b
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEEROPTIONAL(y@2)

    # case c
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEER(z)
    z -> PEEROPTIONAL(y@2)

The peerOptional dependency is included in the peerSet, which would
raise an ERESOLVE conflict at the peerSet generation stage, even though
the peerOptional dependencies will not actually be added to the tree.

To address this, this commit tracks the nodes which are actually
required in the peerSet generation phase, by virtue of being
non-optionally depended upon by a required node in the peerSet.

If a conflict occurs on a node which is not in the required set during
the peerSet generation phase, we ignore it in much the same way that we
would ignore peerSet errors in metadependencies or when --force is used.

Of course, if the peerOptional dependency is _actually_ required, to
avoid a conflict with an invalid resolution present in the tree already,
and there is no suitable placement for it, then ERESOLVE will still be
raised.

Fix: #223
Fix: npm/cli#2667

lib/arborist/build-ideal-tree.js

@@ -826,13 +826,18 @@ 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 parent = edge.peer ? virtualRoot : null
       const dep = vrDep && vrDep.satisfies(edge) ? vrDep
-        : await this[_nodeFromEdge](edge, edge.peer ? virtualRoot : null)
+        : await this[_nodeFromEdge](edge, parent, null, required)
+
       /* istanbul ignore next */
       debug(() => {
         if (!dep)
           throw new Error('no dep??')
       })
+
       tasks.push({edge, dep})
     }
 
@@ -869,7 +874,7 @@ This is a one-time fix-up, please be patient...
 
   // loads a node from an edge, and then loads its peer deps (and their
   // peer deps, on down the line) into a virtual root parent.
-  async [_nodeFromEdge] (edge, parent_, secondEdge = null) {
+  async [_nodeFromEdge] (edge, parent_, secondEdge, required) {
     // create a virtual root node with the same deps as the node that
     // is requesting this one, so that we can get all the peer deps in
     // a context where they're likely to be resolvable.
@@ -900,6 +905,11 @@ This is a one-time fix-up, please be patient...
     // ensure the one we want is the one that's placed
     node.parent = parent
 
+    if (required.has(edge.from) && edge.type !== 'peerOptional' ||
+        secondEdge && (
+          required.has(secondEdge.from) && secondEdge.type !== 'peerOptional'))
+      required.add(node)
+
     // handle otherwise unresolvable dependency nesting loops by
     // creating a symbolic link
     // a1 -> b1 -> a2 -> b2 -> a1 -> ...
@@ -913,7 +923,7 @@ This is a one-time fix-up, please be patient...
     // keep track of the thing that caused this node to be included.
     const src = parent.sourceReference
     this[_peerSetSource].set(node, src)
-    return this[_loadPeerSet](node)
+    return this[_loadPeerSet](node, required)
   }
 
   [_virtualRoot] (node, reuse = false) {
@@ -1058,7 +1068,7 @@ This is a one-time fix-up, please be patient...
   // gets placed first.  In non-strict mode, we behave strictly if the
   // virtual root is based on the root project, and allow non-peer parent
   // deps to override, but throw if no preference can be determined.
-  async [_loadPeerSet] (node) {
+  async [_loadPeerSet] (node, required) {
     const peerEdges = [...node.edgesOut.values()]
       // we typically only install non-optional peers, but we have to
       // factor them into the peerSet so that we can avoid conflicts
@@ -1073,10 +1083,12 @@ This is a one-time fix-up, please be patient...
       const parentEdge = node.parent.edgesOut.get(edge.name)
       const {isProjectRoot, isWorkspace} = node.parent.sourceReference
       const isMine = isProjectRoot || isWorkspace
+      const conflictOK = this[_force] || !isMine && !this[_strictPeerDeps]
+
       if (!edge.to) {
         if (!parentEdge) {
           // easy, just put the thing there
-          await this[_nodeFromEdge](edge, node.parent)
+          await this[_nodeFromEdge](edge, node.parent, null, required)
           continue
         } else {
           // if the parent's edge is very broad like >=1, and the edge in
@@ -1087,14 +1099,16 @@ This is a one-time fix-up, please be patient...
           // a conflict.  this is always a problem in strict mode, never
           // in force mode, and a problem in non-strict mode if this isn't
           // on behalf of our project.  in all such cases, we warn at least.
-          await this[_nodeFromEdge](parentEdge, node.parent, edge)
+          const dep = await this[_nodeFromEdge](parentEdge, node.parent, edge, required)
 
           // hooray! that worked!
           if (edge.valid)
             continue
 
-          // allow it
-          if (this[_force] || !isMine && !this[_strictPeerDeps])
+          // allow it.  either we're overriding, or it's not something
+          // that will be installed by default anyway, and we'll fail when
+          // we get to the point where we need to, if we need to.
+          if (conflictOK || !required.has(dep))
             continue
 
           // problem
@@ -1107,7 +1121,7 @@ This is a one-time fix-up, please be patient...
       // 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 (this[_force] || !isMine && !this[_strictPeerDeps])
+      if (conflictOK)
         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

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

@@ -2454,3 +2454,17 @@ t.test('shrinkwrapped dev/optional deps should not clobber flags', t => {
 
   t.end()
 })
+
+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']
+  t.plan(cases.length)
+  for (const c of cases) {
+    t.test(`case ${c}`, async t => {
+      const path = resolve(base, c)
+      t.matchSnapshot(await printIdeal(path))
+    })
+  }
+})

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

@@ -0,0 +1,5 @@
+# peer optional failures
+
+Cases which incorrectly caused `ERESOLVE` warnings.
+
+[npm/arborist#223](https://github.com/npm/arborist/issues/223)

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

@@ -0,0 +1,9 @@
+# peer optional failure A
+
+```
+root -> (x, y@1)
+x -> PEEROPTIONAL(z)
+z -> PEER(y@2)
+```
+
+[npm/arborist#223](https://github.com/npm/arborist/issues/223)

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

@@ -0,0 +1,8 @@
+{
+  "name": "@isaacs/testing-peer-optional-conflict-a",
+  "version": "1.0.0",
+  "dependencies": {
+    "@isaacs/testing-peer-optional-conflict-a-x": "1",
+    "@isaacs/testing-peer-optional-conflict-a-y": "1"
+  }
+}

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

@@ -0,0 +1,12 @@
+{
+  "name": "@isaacs/testing-peer-optional-conflict-a-x",
+  "version": "1.0.0",
+  "peerDependencies": {
+    "@isaacs/testing-peer-optional-conflict-a-z": "1"
+  },
+  "peerDependenciesMeta": {
+    "@isaacs/testing-peer-optional-conflict-a-z": {
+      "optional": true
+    }
+  }
+}

test/fixtures/peer-optional-eresolve/a/y/1/package.json

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

test/fixtures/peer-optional-eresolve/a/y/2/package.json

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

test/fixtures/peer-optional-eresolve/a/z/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "@isaacs/testing-peer-optional-conflict-a-z",
+  "version": "1.0.0",
+  "peerDependencies": {
+    "@isaacs/testing-peer-optional-conflict-a-y": "2"
+  }
+}

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

@@ -0,0 +1,8 @@
+# peer optional failure b
+
+```
+root -> (x) PEEROPTIONAL(y@1)
+x -> PEEROPTIONAL(y@2)
+```
+
+[npm/arborist#223](https://github.com/npm/arborist/issues/223)

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

@@ -0,0 +1,15 @@
+{
+  "name": "@isaacs/testing-peer-optional-conflict-b",
+  "version": "1.0.0",
+  "dependencies": {
+    "@isaacs/testing-peer-optional-conflict-b-x": "1"
+  },
+  "peerDependencies": {
+    "@isaacs/testing-peer-optional-conflict-b-y": "1"
+  },
+  "peerDependenciesMeta": {
+    "@isaacs/testing-peer-optional-conflict-b-y": {
+      "optional": true
+    }
+  }
+}