From daa5cf0f85f9c9ea58e270c6a18c232d65513244 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 21 Oct 2020 15:08:55 -0700 Subject: [PATCH] Install peerOptionals if explicitly requested, or dev Fix npm/cli#2005 PR-URL: https://github.com/npm/arborist/pull/166 Credit: @isaacs Close: #166 Reviewed-by: @ruyadorno --- lib/add-rm-pkg-deps.js | 2 + lib/arborist/build-ideal-tree.js | 9 +- lib/node.js | 12 +-- ...t-arborist-build-ideal-tree.js-TAP.test.js | 91 +++++++++++++++++++ test/arborist/build-ideal-tree.js | 11 +++ .../peer-optional-installs/package.json | 17 ++++ 6 files changed, 133 insertions(+), 9 deletions(-) create mode 100644 test/fixtures/peer-optional-installs/package.json diff --git a/lib/add-rm-pkg-deps.js b/lib/add-rm-pkg-deps.js index c050bde53..9e4825c52 100644 --- 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 index d0150d238..6591b89a5 100644 --- 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 index bb58b244c..1cf6be2ea 100644 --- 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 index 18c6ce018..5a14b6ae1 100644 --- 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 index 316b0bd46..7b8228ca1 100644 --- 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 new file mode 100644 index 000000000..0b6a3f83b --- /dev/null +++ b/test/fixtures/peer-optional-installs/package.json @@ -0,0 +1,17 @@ +{ + "peerDependencies": { + "abbrev": "", + "wrappy": "" + }, + "devDependencies": { + "wrappy": "" + }, + "peerDependenciesMeta": { + "wrappy": { + "optional": true + }, + "abbrev": { + "optional": true + } + } +}