Install peerOptionals if explicitly requested, or dev

Fix npm/cli#2005
npm · Oct 21, 2020 · 2e90a3b · 2e90a3b
1 parent 66f1a7f
commit 2e90a3b
Show file tree

Hide file tree

Showing 6 changed files with 133 additions and 9 deletions.
diff --git a/lib/add-rm-pkg-deps.js b/lib/add-rm-pkg-deps.js
@@ -15,6 +15,8 @@ const removeFromOthers = (name, type, pkg) => {
       break
     case 'dev':
       others.delete('devDependencies')
+      others.delete('peerDependencies')
+      others.delete('peerDependenciesMeta')
       break
     case 'optional':
       others.delete('optionalDependencies')

diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js
@@ -856,9 +856,12 @@ This is a one-time fix-up, please be patient...
         if (edge.to && edge.to.inShrinkwrap)
           return false
 
-        // If the edge has no destination, that's a problem.
-        if (!edge.to)
-          return edge.type !== 'peerOptional'
+        // If the edge has no destination, that's a problem, unless
+        // if it's peerOptional and not explicitly requested.
+        if (!edge.to) {
+          return edge.type !== 'peerOptional' ||
+            this[_explicitRequests].has(edge.name)
+        }
 
         // If the edge has an error, there's a problem.
         if (!edge.valid)

diff --git a/lib/node.js b/lib/node.js
@@ -502,6 +502,12 @@ class Node {
     this[_loadDepType](this.package.optionalDependencies, 'optional')
     this[_loadDepType](this.package.dependencies, 'prod')
 
+    // Linked targets that are disconnected from the tree are tops,
+    // but don't have a 'path' field, only a 'realpath', because we
+    // don't know their canonical location. We don't need their devDeps.
+    if (this.isTop && this.path)
+      this[_loadDepType](this.package.devDependencies, 'dev')
+
     const pd = this.package.peerDependencies
     if (pd && typeof pd === 'object' && !this.legacyPeerDeps) {
       const pm = this.package.peerDependenciesMeta || {}
@@ -516,12 +522,6 @@ class Node {
       this[_loadDepType](peerDependencies, 'peer')
       this[_loadDepType](peerOptional, 'peerOptional')
     }
-
-    // Linked targets that are disconnected from the tree are tops,
-    // but don't have a 'path' field, only a 'realpath', because we
-    // don't know their canonical location. We don't need their devDeps.
-    if (this.isTop && this.path)
-      this[_loadDepType](this.package.devDependencies, 'dev')
   }
 
   [_loadDepType] (obj, type) {

diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js
@@ -65110,6 +65110,97 @@ Node {
 }
 `
 
+exports[`test/arborist/build-ideal-tree.js TAP peerOptionals that are devDeps or explicit request > should install the abbrev dep 1`] = `
+Node {
+  "children": Map {
+    "abbrev" => Node {
+      "edgesIn": Set {
+        Edge {
+          "from": "",
+          "name": "abbrev",
+          "spec": "",
+          "type": "peerOptional",
+        },
+      },
+      "location": "node_modules/abbrev",
+      "name": "abbrev",
+      "optional": true,
+      "peer": true,
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+    },
+    "wrappy" => Node {
+      "dev": true,
+      "edgesIn": Set {
+        Edge {
+          "from": "",
+          "name": "wrappy",
+          "spec": "",
+          "type": "dev",
+        },
+      },
+      "location": "node_modules/wrappy",
+      "name": "wrappy",
+      "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz",
+    },
+  },
+  "edgesOut": Map {
+    "abbrev" => Edge {
+      "name": "abbrev",
+      "spec": "",
+      "to": "node_modules/abbrev",
+      "type": "peerOptional",
+    },
+    "wrappy" => Edge {
+      "name": "wrappy",
+      "spec": "",
+      "to": "node_modules/wrappy",
+      "type": "dev",
+    },
+  },
+  "location": "",
+  "name": "peer-optional-installs",
+  "resolved": null,
+}
+`
+
+exports[`test/arborist/build-ideal-tree.js TAP peerOptionals that are devDeps or explicit request > should install the wrappy dep, and not remove from peerDeps 1`] = `
+Node {
+  "children": Map {
+    "wrappy" => Node {
+      "dev": true,
+      "edgesIn": Set {
+        Edge {
+          "from": "",
+          "name": "wrappy",
+          "spec": "",
+          "type": "dev",
+        },
+      },
+      "location": "node_modules/wrappy",
+      "name": "wrappy",
+      "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz",
+    },
+  },
+  "edgesOut": Map {
+    "abbrev" => Edge {
+      "name": "abbrev",
+      "spec": "",
+      "to": null,
+      "type": "peerOptional",
+    },
+    "wrappy" => Edge {
+      "name": "wrappy",
+      "spec": "",
+      "to": "node_modules/wrappy",
+      "type": "dev",
+    },
+  },
+  "location": "",
+  "name": "peer-optional-installs",
+  "resolved": null,
+}
+`
+
 exports[`test/arborist/build-ideal-tree.js TAP push conflicted peer deps deeper in to the tree to solve > must match snapshot 1`] = `
 Node {
   "children": Map {

diff --git a/test/arborist/build-ideal-tree.js b/test/arborist/build-ideal-tree.js
@@ -2114,3 +2114,14 @@ t.test('carbonium eslint conflicts', async t => {
     ],
   }))
 })
+
+t.test('peerOptionals that are devDeps or explicit request', async t => {
+  const path = resolve(fixtures, 'peer-optional-installs')
+  t.matchSnapshot(await printIdeal(path, {
+    add: ['abbrev'],
+  }), 'should install the abbrev dep')
+  t.matchSnapshot(await printIdeal(path, {
+    add: ['wrappy'],
+    saveType: 'dev',
+  }), 'should install the wrappy dep, and not remove from peerDeps')
+})
diff --git a/test/fixtures/peer-optional-installs/package.json b/test/fixtures/peer-optional-installs/package.json
@@ -0,0 +1,17 @@
+{
+  "peerDependencies": {
+    "abbrev": "",
+    "wrappy": ""
+  },
+  "devDependencies": {
+    "wrappy": ""
+  },
+  "peerDependenciesMeta": {
+    "wrappy": {
+      "optional": true
+    },
+    "abbrev": {
+      "optional": true
+    }
+  }
+}