Skip to content
Permalink
Browse files

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 <jv@jviotti.com>
  • Loading branch information...
jviotti committed Sep 14, 2018
1 parent 59e5056 commit 571d90f82527e2a2e4fc88d160b42c0f1143cf01
Showing with 45 additions and 2 deletions.
  1. +2 −1 lib/install/diff-trees.js
  2. +18 −0 lib/install/is-dev.js
  3. +25 −1 test/tap/install-cli-only-development.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
})
@@ -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)
})
}
@@ -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'),

0 comments on commit 571d90f

Please sign in to comment.
You can’t perform that action at this time.