From 4fb34651d8bca9164eb3aba96700d021b6165d26 Mon Sep 17 00:00:00 2001
From: Juan Cruz Viotti <jv@jviotti.com>
Date: Mon, 4 Mar 2019 19:48:41 -0300
Subject: [PATCH] install: Don't omit any deps of dev deps if --only=dev (#69)

* install: Don't omit any deps of dev deps if --only=dev

At the moment, npm will NOT install a dependency that is both a
production and a development dependency in the same tree if
`--only=dev`.

Consider the following scenario:

- `package1` has `prod-dependency` in `dependencies`
- `package1` has `dev-dependency` in `devDependencies`
- `prod-dependency` has `sub-dependency` in `dependencies`
- `dev-dependency` has `sub-dependency` in `dependencies`

So both `prod-dependency` and `dev-dependency` directly depend on
`sub-dependency`. Since `sub-dependency` is required by at least one
production dependency, then npm won't consider it as "only a dev
dependency", and therefore running `npm install --only=dev` will result
in the following tree:

```
package1/
    node_modules/
        dev-dependency
```

Notice that `sub-dependency` has ben completely omitted from the tree,
even though `dev-dependency` clearly requires it, and therefore it will
not work.

This commit makes `--only=dev` always install required dependencies of
dev dependencies, so you never end up with a broken tree.

For a more real-world reproducible example, try to run `npm install
--only=dev` in
https://github.com/resin-io/etcher/tree/a338c6e60a10aad00008febfe92e2556dcf586c6.
The resulting tree will be completely broken, but we would not have
identified this if several install scripts wouldn't fail as a result.

PR-URL: https://github.com/npm/cli/pull/69
Credit: @jviotti
Reviewed-By: @iarna
---
 lib/install/diff-trees.js                |  4 +++-
 lib/install/is-dev.js                    | 18 ++++++++++++++++
 test/tap/install-cli-only-development.js | 26 +++++++++++++++++++++++-
 3 files changed, 46 insertions(+), 2 deletions(-)
 create mode 100644 lib/install/is-dev.js

diff --git a/lib/install/diff-trees.js b/lib/install/diff-trees.js
index 346846fdc0ffe..7ff2cb89df539 100644
--- a/lib/install/diff-trees.js
+++ b/lib/install/diff-trees.js
@@ -4,6 +4,7 @@ var validate = require('aproba')
 var npa = require('npm-package-arg')
 var flattenTree = require('./flatten-tree.js')
 var isOnlyDev = require('./is-only-dev.js')
+var isDev = require('./is-dev.js')
 var log = require('npmlog')
 var path = require('path')
 var ssri = require('ssri')
@@ -250,10 +251,11 @@ function filterActions (differences) {
   log.silly('diff-trees', 'filtering actions:', 'includeDev', includeDev, 'includeProd', includeProd, 'includeOpt', includeOpt)
   return differences.filter((diff) => {
     const pkg = diff[1]
+    const pkgIsDev = isDev(pkg)
     const pkgIsOnlyDev = isOnlyDev(pkg)
     const pkgIsOnlyOpt = isOnlyOptional(pkg)
     if (!includeProd && pkgIsOnlyDev) return true
-    if (includeDev && pkgIsOnlyDev) return true
+    if (includeDev && pkgIsDev) return true // if we used isOnlyDev here we'd exclude mixed prod/dev packages
     if (includeProd && !pkgIsOnlyDev && (includeOpt || !pkgIsOnlyOpt)) return true
     return false
   })
diff --git a/lib/install/is-dev.js b/lib/install/is-dev.js
new file mode 100644
index 0000000000000..e2197d373f3e5
--- /dev/null
+++ b/lib/install/is-dev.js
@@ -0,0 +1,18 @@
+'use strict'
+module.exports = isDev
+
+// Returns true if the module `node` is a dev dependency itself or a
+// dependency of some dev dependency higher in the hierarchy.
+function isDev (node, seen) {
+  if (!seen) seen = new Set()
+  if (seen.has(node.package.name)) return true
+
+  seen.add(node.package.name)
+  if (!node.requiredBy || node.requiredBy.length === 0 || node.package._development || node.isTop) {
+    return !!node.package._development
+  }
+
+  return node.requiredBy.some((parent) => {
+    return isDev(parent, seen)
+  })
+}
diff --git a/test/tap/install-cli-only-development.js b/test/tap/install-cli-only-development.js
index ff9d05f547f26..21ce2665c0009 100644
--- a/test/tap/install-cli-only-development.js
+++ b/test/tap/install-cli-only-development.js
@@ -28,12 +28,24 @@ var json = {
 var dependency = {
   name: 'dependency',
   description: 'fixture',
-  version: '0.0.0'
+  version: '0.0.0',
+  dependencies: {
+    'sub-dependency': 'file:../sub-dependency'
+  }
 }
 
 var devDependency = {
   name: 'dev-dependency',
   description: 'fixture',
+  version: '0.0.0',
+  dependencies: {
+    'sub-dependency': 'file:../sub-dependency'
+  }
+}
+
+var subDependency = {
+  name: 'sub-dependency',
+  description: 'fixture',
   version: '0.0.0'
 }
 
@@ -57,6 +69,12 @@ test('\'npm install --only=development\' should only install devDependencies', f
       existsSync(path.resolve(pkg, 'node_modules/dependency/package.json')),
       'dependency was NOT installed'
     )
+    t.ok(
+      JSON.parse(fs.readFileSync(
+        path.resolve(pkg, 'node_modules/sub-dependency/package.json'), 'utf8')
+      ),
+      'subDependency was installed'
+    )
     t.end()
   })
 })
@@ -101,6 +119,12 @@ function setup () {
     JSON.stringify(devDependency, null, 2)
   )
 
+  mkdirp.sync(path.join(pkg, 'sub-dependency'))
+  fs.writeFileSync(
+    path.join(pkg, 'sub-dependency', 'package.json'),
+    JSON.stringify(subDependency, null, 2)
+  )
+
   mkdirp.sync(path.join(pkg, 'node_modules'))
   fs.writeFileSync(
     path.join(pkg, 'package.json'),