From 571d90f82527e2a2e4fc88d160b42c0f1143cf01 Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Fri, 14 Sep 2018 12:52:37 +0100 Subject: [PATCH 1/2] 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. Signed-off-by: Juan Cruz Viotti --- lib/install/diff-trees.js | 3 ++- lib/install/is-dev.js | 18 ++++++++++++++++ test/tap/install-cli-only-development.js | 26 +++++++++++++++++++++++- 3 files changed, 45 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..0d2d4c744b171 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') @@ -253,7 +254,7 @@ function filterActions (differences) { const pkgIsOnlyDev = isOnlyDev(pkg) const pkgIsOnlyOpt = isOnlyOptional(pkg) if (!includeProd && pkgIsOnlyDev) return true - if (includeDev && pkgIsOnlyDev) return true + if (includeDev && isDev(pkg)) return true 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'), From 17c8938396ac9e2cef2f820430969185a70208d4 Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Mon, 4 Mar 2019 14:46:42 -0800 Subject: [PATCH 2/2] Update diff-trees.js --- lib/install/diff-trees.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/install/diff-trees.js b/lib/install/diff-trees.js index 0d2d4c744b171..7ff2cb89df539 100644 --- a/lib/install/diff-trees.js +++ b/lib/install/diff-trees.js @@ -251,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 && isDev(pkg)) 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 })