Fix npm#10727 - Consider devDependencies when deciding whether to hoi…

…st a package

Fixes several issues that were causing devDependencies to clobber
transitive dependencies.

When prod & dev dependencies are installed in the same batch, both
groups of dependencies are now processed together so that overlapping
transitive dependencies are properly deduplicated.

Fixes npm#10727 npm#11062 npm#12654 npm#10277 npm#11766 npm#11043

lib/install.js

@@ -119,6 +119,7 @@ var copyTree = require('./install/copy-tree.js')
 var readShrinkwrap = require('./install/read-shrinkwrap.js')
 var recalculateMetadata = require('./install/deps.js').recalculateMetadata
 var loadDeps = require('./install/deps.js').loadDeps
+var loadAllDeps = require('./install/deps.js').loadAllDeps
 var loadDevDeps = require('./install/deps.js').loadDevDeps
 var getAllMetadata = require('./install/deps.js').getAllMetadata
 var loadRequestedDeps = require('./install/deps.js').loadRequestedDeps
@@ -382,11 +383,13 @@ Installer.prototype.loadAllDepsIntoIdealTree = function (cb) {
     steps.push([validateArgs, this.idealTree, this.args])
     steps.push([loadRequestedDeps, this.args, this.idealTree, saveDeps, cg.newGroup('loadRequestedDeps')])
   } else {
-    if (this.prod) {
+    if (this.prod && this.dev) {
+      steps.push(
+        [loadAllDeps, this.idealTree, cg.newGroup('loadAllDeps')])
+    } else if (this.prod) {
       steps.push(
         [loadDeps, this.idealTree, cg.newGroup('loadDeps')])
-    }
-    if (this.dev) {
+    } else if (this.dev) {
       steps.push(
         [loadDevDeps, this.idealTree, cg.newGroup('loadDevDeps')])
     }

lib/install/deps.js

@@ -364,6 +364,28 @@ function andHandleOptionalErrors (log, tree, name, done) {
 }
 
 // Load any missing dependencies in the given tree
+exports.loadAllDeps = loadAllDeps
+function loadAllDeps (tree, log, next) {
+  validate('OOF', arguments)
+  if (tree.loaded || (tree.parent && tree.parent.failed)) return andFinishTracker.now(log, next)
+  if (tree.parent) tree.loaded = true
+  if (!tree.package.dependencies) tree.package.dependencies = {}
+  if (!tree.package.devDependencies) tree.package.devDependencies = {}
+
+  var deps = union(Object.keys(tree.package.dependencies), Object.keys(tree.package.devDependencies))
+  asyncMap(deps, function (dep, done) {
+    var version = tree.package.dependencies[dep] || tree.package.devDependencies[dep]
+    if (tree.package.optionalDependencies &&
+        tree.package.optionalDependencies[dep] &&
+        !npm.config.get('optional')) {
+      return done()
+    }
+
+    addDependency(dep, version, tree, log.newGroup('loadDep:' + dep), andHandleOptionalErrors(log, tree, dep, done))
+  }, andForEachChild(loadDeps, andFinishTracker(log, next)))
+}
+
+// Load any missing production dependencies in the given tree
 exports.loadDeps = loadDeps
 function loadDeps (tree, log, next) {
   validate('OOF', arguments)
@@ -613,8 +635,12 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr
   // if this tree location requested the same module then we KNOW it
   // isn't compatible because if it were findRequirement would have
   // found that version.
-  var deps = tree.package.dependencies || {}
-  if (!tree.removed && requiredBy !== tree && deps[pkg.name]) {
+  var deps = Object.keys(tree.package.dependencies || {})
+  if (!tree.parent && (npm.config.get('dev') || !npm.config.get('production'))) {
+    deps = union(deps, Object.keys(tree.package.devDependencies || {}))
+  }
+
+  if (!tree.removed && requiredBy !== tree && deps.indexOf(pkg.name) !== -1) {
     return null
   }
 

test/tap/shrinkwrap-shared-dev-dependency.js

@@ -21,6 +21,7 @@ test("shrinkwrap doesn't strip out the shared dependency", function (t) {
 
       npm.install('.', function (err) {
         if (err) return t.fail(err)
+        npm.config.set('dev', true) // npm install unsets this
 
         npm.commands.shrinkwrap([], true, function (err, results) {
           if (err) return t.fail(err)

test/tap/unit-deps-earliestInstallable.js

@@ -0,0 +1,58 @@
+'use strict'
+var test = require('tap').test
+var requireInject = require('require-inject')
+
+// we're just mocking to avoid having to call `npm.load`
+var deps = requireInject('../../lib/install/deps.js', {
+  '../../lib/npm.js': {
+    config: {
+      get: function (val) { return (val === 'global-style' || val === 'legacy-bundling') ? false : 'mock' }
+    }
+  }
+})
+
+var earliestInstallable = deps.earliestInstallable
+
+test('earliestInstallable should consider devDependencies', function (t) {
+  var dep1 = {
+    children: [],
+    package: {
+      name: 'dep1',
+      dependencies: { dep2: '2.0.0' }
+    }
+  }
+
+  // a library required by the base package
+  var dep2 = {
+    package: {
+      name: 'dep2',
+      version: '1.0.0'
+    }
+  }
+
+  // an incompatible verson of dep2. required by dep1
+  var dep2a = {
+    package: {
+      name: 'dep2',
+      version: '2.0.0'
+    },
+    parent: dep1
+  }
+
+  var pkg = {
+    children: [dep1],
+    package: {
+      name: 'pkg',
+      dependencies: { dep1: '1.0.0' },
+      devDependencies: { dep2: '1.0.0' }
+    }
+  }
+
+  dep1.parent = pkg
+  dep2a.parent = dep1
+  dep2.parent = pkg
+
+  var earliest = earliestInstallable(dep1, dep1, dep2a.package)
+  t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
+  t.end()
+})