deps: When we replace/upgrade a dep, remove all its deps too

PR-URL: #9929

lib/install/deps.js

@@ -133,6 +133,16 @@ function addRequiredDep (tree, child) {
   return true
 }
 
+function removeObsoleteDep (child) {
+  if (child.removed) return
+  child.removed = true
+  var requires = child.requires || []
+  requires.forEach(function (requirement) {
+    requirement.requiredBy = requirement.requiredBy.filter(function (reqBy) { reqBy !== child })
+    if (requirement.requiredBy.length === 0) removeObsoleteDep(requirement)
+  })
+}
+
 function matchingDep (tree, name) {
   if (tree.package.dependencies && tree.package.dependencies[name]) return tree.package.dependencies[name]
   if (tree.package.devDependencies && tree.package.devDependencies[name]) return tree.package.devDependencies[name]
@@ -178,10 +188,6 @@ exports.loadRequestedDeps = function (args, tree, saveToDependencies, log, next)
   validate('AOOF', [args, tree, log, next])
   asyncMap(args, function (pkg, done) {
     var depLoaded = andAddParentToErrors(tree, done)
-    var pkgName = moduleName(pkg)
-    tree.children = tree.children.filter(function (child) {
-      return moduleName(child) !== pkgName
-    })
     resolveWithNewModule(pkg, tree, log.newGroup('loadRequestedDeps'), iferr(depLoaded, function (child, tracker) {
       validate('OO', arguments)
       if (npm.config.get('global')) {
@@ -464,7 +470,8 @@ function resolveWithNewModule (pkg, tree, log, next) {
         isLink: tree.isLink
       })
 
-      replaceModule(parent, 'children', child)
+      var replaced = replaceModule(parent, 'children', child)
+      if (replaced) removeObsoleteDep(replaced)
       addRequiredDep(tree, child)
       pkg._location = flatNameFromTree(child)
 
@@ -511,7 +518,7 @@ function validateAllPeerDeps (tree, onInvalid, seen) {
 var findRequirement = exports.findRequirement = function (tree, name, requested) {
   validate('OSO', arguments)
   var nameMatch = function (child) {
-    return moduleName(child) === name && child.parent
+    return moduleName(child) === name && child.parent && !child.removed
   }
   var versionMatch = function (child) {
     return doesChildVersionMatch(child, requested)
@@ -539,15 +546,16 @@ var findRequirement = exports.findRequirement = function (tree, name, requested)
 // If it is, then it's the level below where its installed.
 var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg) {
   validate('OOO', arguments)
-  var nameMatch = function (child) {
-    return moduleName(child) === pkg.name
-  }
 
-  if (tree.children.some(nameMatch)) return null
+  function undeletedModuleMatches (child) {
+    return !child.removed && moduleName(child) === pkg.name
+  }
+  if (tree.children.some(undeletedModuleMatches)) return null
 
   // If any of the children of this tree have conflicting
   // binaries then we need to decline to install this package here.
   var binaryMatches = tree.children.some(function (child) {
+    if (child.removed) return false
     return Object.keys(child.package.bin || {}).some(function (bin) {
       return pkg.bin && pkg.bin[bin]
     })
@@ -557,7 +565,8 @@ 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.
-  if (requiredBy !== tree && tree.package.dependencies && tree.package.dependencies[pkg.name]) {
+  var deps = tree.package.dependencies || {}
+  if (!tree.removed && requiredBy !== tree && deps[pkg.name]) {
     return null
   }
 

test/tap/unit-deps-replaceModule.js

@@ -0,0 +1,50 @@
+'use strict'
+var test = require('tap').test
+var npm = require('../../lib/npm')
+
+test('setup', function (t) {
+  npm.load({}, t.done)
+})
+
+test('replaceModule', function (t) {
+  var replaceModule = require('../../lib/install/deps').test.replaceModule
+  var mods = []
+  for (var ii = 0; ii < 10; ++ii) {
+    mods.push({package: {name: ii}})
+  }
+
+  var test = {}
+  test.A = mods.slice(0, 4)
+  replaceModule(test, 'A', mods[2])
+  t.isDeeply(test.A, mods.slice(0, 4), 'replacing an existing module leaves the order alone')
+  replaceModule(test, 'A', mods[7])
+  t.isDeeply(test.A, mods.slice(0, 4).concat(mods[7]), 'replacing a new module appends')
+
+  test.B = mods.slice(0, 4)
+  var replacement = {package: {name: 1}, isReplacement: true}
+  replaceModule(test, 'B', replacement)
+  t.isDeeply(test.B, [mods[0], replacement, mods[2], mods[3]], 'replacing existing module swaps out for the new version')
+
+  replaceModule(test, 'C', mods[7])
+  t.isDeeply(test.C, [mods[7]], 'replacing when the key does not exist yet, causes its creation')
+  t.end()
+})
+
+test('replaceModuleName', function (t) {
+  var replaceModuleName = require('../../lib/install/deps').test.replaceModuleName
+  var mods = []
+  for (var ii = 0; ii < 10; ++ii) {
+    mods.push('pkg' + ii)
+  }
+
+  var test = {}
+  test.A = mods.slice(0, 4)
+  replaceModuleName(test, 'A', mods[2])
+  t.isDeeply(test.A, mods.slice(0, 4), 'replacing an existing module leaves the order alone')
+  replaceModuleName(test, 'A', mods[7])
+  t.isDeeply(test.A, mods.slice(0, 4).concat(mods[7]), 'replacing a new module appends')
+
+  replaceModuleName(test, 'C', mods[7])
+  t.isDeeply(test.C, [mods[7]], 'replacing when the key does not exist yet, causes its creation')
+  t.end()
+})