uninstall: When removing direct deps don't remove if also transitive dep

6 files changed, 113 insertions, 89 deletions

diff --git a/lib/install.js b/lib/install.js
index 12868db4d..20d3d0f96 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -137,10 +137,10 @@ var doReverseSerialActions = require('./install/actions.js').doReverseSerial
 var doParallelActions = require('./install/actions.js').doParallel
 var doOneAction = require('./install/actions.js').doOne
 var removeObsoleteDep = require('./install/deps.js').removeObsoleteDep
+var removeExtraneous = require('./install/deps.js').removeExtraneous
 var packageId = require('./utils/package-id.js')
 var moduleName = require('./utils/module-name.js')
 var errorMessage = require('./utils/error-message.js')
-var removeDeps = require('./install/deps.js').removeDeps
 var isExtraneous = require('./install/is-extraneous.js')
 
 function unlockCB (lockPath, name, cb) {
@@ -386,9 +386,9 @@ Installer.prototype.loadIdealTree = function (cb) {
 
 Installer.prototype.pruneIdealTree = function (cb) {
   var toPrune = this.idealTree.children
-    .filter((n) => !n.fromShrinkwrap && isExtraneous(n))
+    .filter((n) => !n.fakeChild && isExtraneous(n))
     .map((n) => ({name: moduleName(n)}))
-  return removeDeps(toPrune, this.idealTree, null, log.newGroup('pruneDeps'), cb)
+  return removeExtraneous(toPrune, this.idealTree, cb)
 }
 
 Installer.prototype.loadAllDepsIntoIdealTree = function (cb) {
@@ -400,26 +400,25 @@ Installer.prototype.loadAllDepsIntoIdealTree = function (cb) {
   var installNewModules = !!this.args.length
   var steps = []
 
+  const depsToPreload = Object.assign({},
+    this.dev ? this.idealTree.package.devDependencies : {},
+    this.prod ? this.idealTree.package.dependencies : {}
+  )
   if (installNewModules) {
     steps.push([validateArgs, this.idealTree, this.args])
     steps.push([loadRequestedDeps, this.args, this.idealTree, saveDeps, cg.newGroup('loadRequestedDeps')])
-  } else {
-    const depsToPreload = Object.assign({},
-      this.dev ? this.idealTree.package.devDependencies : {},
-      this.prod ? this.idealTree.package.dependencies : {}
-    )
-    if (this.prod || this.dev) {
-      steps.push(
-        [prefetchDeps, this.idealTree, depsToPreload, cg.newGroup('prefetchDeps')])
-    }
-    if (this.prod) {
-      steps.push(
-        [loadDeps, this.idealTree, cg.newGroup('loadDeps')])
-    }
-    if (this.dev) {
-      steps.push(
-        [loadDevDeps, this.idealTree, cg.newGroup('loadDevDeps')])
-    }
+  }
+  if (this.prod || this.dev) {
+    steps.push(
+      [prefetchDeps, this.idealTree, depsToPreload, cg.newGroup('prefetchDeps')])
+  }
+  if (this.prod) {
+    steps.push(
+      [loadDeps, this.idealTree, cg.newGroup('loadDeps')])
+  }
+  if (this.dev) {
+    steps.push(
+      [loadDevDeps, this.idealTree, cg.newGroup('loadDevDeps')])
   }
   steps.push(
     [loadExtraneous.andResolveDeps, this.idealTree, cg.newGroup('loadExtraneous')])
@@ -581,7 +580,14 @@ Installer.prototype.saveToDependencies = function (cb) {
   validate('F', arguments)
   if (this.failing) return cb()
   log.silly('install', 'saveToDependencies')
-  saveRequested(this.args, this.idealTree, cb)
+  if (
+    this.differences.length ||
+    (this.idealTree.removedChildren || []).length
+  ) {
+    saveRequested(this.idealTree, cb)
+  } else {
+    cb()
+  }
 }
 
 Installer.prototype.readGlobalPackageData = function (cb) {
@@ -693,21 +699,22 @@ Installer.prototype.printInstalled = function (cb) {
   validate('F', arguments)
   if (this.failing) return cb()
   log.silly('install', 'printInstalled')
+  const diffs = this.differences.concat((this.idealTree.removedChildren || []).map((r) => ['remove', r]))
   if (npm.config.get('json')) {
-    return this.printInstalledForJSON(cb)
+    return this.printInstalledForJSON(diffs, cb)
   } else if (npm.config.get('parseable')) {
-    return this.printInstalledForParseable(cb)
+    return this.printInstalledForParseable(diffs, cb)
   } else {
-    return this.printInstalledForHuman(cb)
+    return this.printInstalledForHuman(diffs, cb)
   }
 }
 
-Installer.prototype.printInstalledForHuman = function (cb) {
+Installer.prototype.printInstalledForHuman = function (diffs, cb) {
   var removed = 0
   var added = 0
   var updated = 0
   var moved = 0
-  this.differences.forEach(function (action) {
+  diffs.forEach(function (action) {
     var mutation = action[0]
     if (mutation === 'remove') {
       ++removed
@@ -743,7 +750,7 @@ Installer.prototype.printInstalledForHuman = function (cb) {
   }
 }
 
-Installer.prototype.printInstalledForJSON = function (cb) {
+Installer.prototype.printInstalledForJSON = function (diffs, cb) {
   var result = {
     added: [],
     removed: [],
@@ -764,7 +771,7 @@ Installer.prototype.printInstalledForJSON = function (cb) {
     }
     result.warnings.push(message)
   })
-  this.differences.forEach(function (action) {
+  diffs.forEach(function (action) {
     var mutation = action[0]
     var child = action[1]
     var record = recordAction(action)
@@ -805,9 +812,9 @@ Installer.prototype.printInstalledForJSON = function (cb) {
   }
 }
 
-Installer.prototype.printInstalledForParseable = function (cb) {
+Installer.prototype.printInstalledForParseable = function (diffs, cb) {
   var self = this
-  this.differences.forEach(function (action) {
+  diffs.forEach(function (action) {
     var mutation = action[0]
     var child = action[1]
     if (mutation === 'move') {
diff --git a/lib/install/deps.js b/lib/install/deps.js
index 9300c6838..4137a5993 100644
--- a/lib/install/deps.js
+++ b/lib/install/deps.js
@@ -285,26 +285,41 @@ function noModuleNameMatches (name) {
 
 // while this implementation does not require async calling, doing so
 // gives this a consistent interface with loadDeps et al
-exports.removeDeps = function (args, tree, saveToDependencies, log, next) {
-  validate('AOOF', [args, tree, log, next])
-  args.forEach(function (pkg) {
+exports.removeDeps = function (args, tree, saveToDependencies, next) {
+  validate('AOSF|AOZF', [args, tree, saveToDependencies, next])
+  for (let pkg of args) {
     var pkgName = moduleName(pkg)
     var toRemove = tree.children.filter(moduleNameMatches(pkgName))
     var pkgToRemove = toRemove[0] || createChild({package: {name: pkgName}})
-    if (tree.isTop) {
-      if (saveToDependencies) {
-        pkgToRemove.save = getSaveType(tree, pkg)
+    if (tree.isTop && saveToDependencies) {
+      pkgToRemove.save = getSaveType(tree, pkg)
+      let existing = false
+      if (tree.package[pkgToRemove.save][pkgName]) {
+        existing = true
         delete tree.package[pkgToRemove.save][pkgName]
-        if (pkgToRemove.save === 'optionalDependencies') {
+      }
+      if (existing && pkgToRemove.save === 'optionalDependencies') {
+        if (tree.package.dependencies[pkgName]) {
+          existing = true
           delete tree.package.dependencies[pkgName]
         }
-        replaceModuleByPath(tree, 'removed', pkgToRemove)
       }
-      pkgToRemove.requiredBy = pkgToRemove.requiredBy.filter((parent) => parent !== tree)
+      if (existing) {
+        replaceModuleByPath(tree, 'removedChildren', pkgToRemove)
+      }
     }
-    if (pkgToRemove.requiredBy.length === 0) removeObsoleteDep(pkgToRemove)
-  })
-  log.finish()
+    pkgToRemove.requiredBy = pkgToRemove.requiredBy.filter((parent) => parent !== tree)
+  }
+  next()
+}
+exports.removeExtraneous = function (args, tree, next) {
+  for (let pkg of args) {
+    var pkgName = moduleName(pkg)
+    var toRemove = tree.children.filter(moduleNameMatches(pkgName))
+    if (toRemove.length) {
+      removeObsoleteDep(toRemove[0])
+    }
+  }
   next()
 }
 
diff --git a/lib/install/inflate-shrinkwrap.js b/lib/install/inflate-shrinkwrap.js
index 9878b0f19..d9f455517 100644
--- a/lib/install/inflate-shrinkwrap.js
+++ b/lib/install/inflate-shrinkwrap.js
@@ -118,6 +118,7 @@ function makeFakeChild (name, topPath, tree, sw, requested) {
       parent: tree,
       children: pkg._bundled || [],
       fromShrinkwrap: true,
+      fakeChild: true,
       fromBundle: sw.bundled ? tree.fromBundle || tree : null,
       path: childPath(tree.path, pkg),
       realpath: childPath(tree.realpath, pkg),
diff --git a/lib/install/save.js b/lib/install/save.js
index 6eba80eb7..e214a5428 100644
--- a/lib/install/save.js
+++ b/lib/install/save.js
@@ -19,9 +19,9 @@ const writeFileAtomic = require('write-file-atomic')
 // if the -S|--save option is specified, then write installed packages
 // as dependencies to a package.json file.
 
-exports.saveRequested = function (args, tree, andReturn) {
-  validate('AOF', arguments)
-  savePackageJson(args, tree, andWarnErrors(andSaveShrinkwrap(tree, andReturn)))
+exports.saveRequested = function (tree, andReturn) {
+  validate('OF', arguments)
+  savePackageJson(tree, andWarnErrors(andSaveShrinkwrap(tree, andReturn)))
 }
 
 function andSaveShrinkwrap (tree, andReturn) {
@@ -46,10 +46,8 @@ function saveShrinkwrap (tree, next) {
   createShrinkwrap(tree, {silent: false}, next)
 }
 
-function savePackageJson (args, tree, next) {
-  validate('AOF', arguments)
-  if (!args || !args.length) { return next() }
-
+function savePackageJson (tree, next) {
+  validate('OF', arguments)
   var saveBundle = npm.config.get('save-bundle')
 
   // each item in the tree is a top-level thing that should be saved
diff --git a/lib/prune.js b/lib/prune.js
index 39d1c8ffb..9ca17ae29 100644
--- a/lib/prune.js
+++ b/lib/prune.js
@@ -59,7 +59,7 @@ Pruner.prototype.loadAllDepsIntoIdealTree = function (cb) {
   var toPrune = this.idealTree.children.filter(shouldPrune).map(getModuleName).filter(matchesArg).map(nameObj)
 
   steps.push(
-    [removeDeps, toPrune, this.idealTree, null, cg.newGroup('removeDeps')],
+    [removeDeps, toPrune, this.idealTree, null],
     [loadExtraneous, this.idealTree, cg.newGroup('loadExtraneous')])
   chain(steps, cb)
 }
diff --git a/lib/uninstall.js b/lib/uninstall.js
index 32b8e2b30..3fa711257 100644
--- a/lib/uninstall.js
+++ b/lib/uninstall.js
@@ -2,20 +2,17 @@
 // remove a package.
 
 module.exports = uninstall
-module.exports.Uninstaller = Uninstaller
 
-var util = require('util')
-var path = require('path')
-var validate = require('aproba')
-var chain = require('slide').chain
-var readJson = require('read-package-json')
-var npm = require('./npm.js')
-var Installer = require('./install.js').Installer
-var getSaveType = require('./install/save.js').getSaveType
-var removeDeps = require('./install/deps.js').removeDeps
-var loadExtraneous = require('./install/deps.js').loadExtraneous
-var log = require('npmlog')
-var usage = require('./utils/usage')
+const path = require('path')
+const validate = require('aproba')
+const readJson = require('read-package-json')
+const iferr = require('iferr')
+const npm = require('./npm.js')
+const Installer = require('./install.js').Installer
+const getSaveType = require('./install/save.js').getSaveType
+const removeDeps = require('./install/deps.js').removeDeps
+const log = require('npmlog')
+const usage = require('./utils/usage')
 
 uninstall.usage = usage(
   'uninstall',
@@ -27,17 +24,18 @@ uninstall.completion = require('./utils/completion/installed-shallow.js')
 function uninstall (args, cb) {
   validate('AF', arguments)
   // the /path/to/node_modules/..
-  var dryrun = !!npm.config.get('dry-run')
+  const dryrun = !!npm.config.get('dry-run')
 
   if (args.length === 1 && args[0] === '.') args = []
-  args = args.filter(function (a) {
-    return path.resolve(a) !== where
-  })
 
-  var where = npm.config.get('global') || !args.length
+  const where = npm.config.get('global') || !args.length
             ? path.resolve(npm.globalDir, '..')
             : npm.prefix
 
+  args = args.filter(function (a) {
+    return path.resolve(a) !== where
+  })
+
   if (args.length) {
     new Uninstaller(where, dryrun, args).run(cb)
   } else {
@@ -50,29 +48,34 @@ function uninstall (args, cb) {
   }
 }
 
-function Uninstaller (where, dryrun, args) {
-  validate('SBA', arguments)
-  Installer.call(this, where, dryrun, args)
-}
-util.inherits(Uninstaller, Installer)
+class Uninstaller extends Installer {
+  constructor (where, dryrun, args) {
+    super(where, dryrun, args)
+    this.remove = []
+  }
 
-Uninstaller.prototype.loadArgMetadata = function (next) {
-  this.args = this.args.map(function (arg) { return {name: arg} })
-  next()
-}
+  loadArgMetadata (next) {
+    this.remove = this.args.map(function (arg) { return {name: arg} })
+    this.args = []
+    next()
+  }
 
-Uninstaller.prototype.loadAllDepsIntoIdealTree = function (cb) {
-  validate('F', arguments)
-  log.silly('uninstall', 'loadAllDepsIntoIdealTree')
-  var saveDeps = getSaveType()
+  loadAllDepsIntoIdealTree (cb) {
+    validate('F', arguments)
+    log.silly('uninstall', 'loadAllDepsIntoIdealTree')
+    const saveDeps = getSaveType()
 
-  var cg = this.progress['loadIdealTree:loadAllDepsIntoIdealTree']
-  var steps = []
-  steps.push(
-    [removeDeps, this.args, this.idealTree, saveDeps, cg.newGroup('removeDeps')],
-    [loadExtraneous, this.idealTree, cg.newGroup('loadExtraneous')])
-  chain(steps, cb)
+    super.loadAllDepsIntoIdealTree(iferr(cb, () => {
+      removeDeps(this.remove, this.idealTree, saveDeps, (err) => {
+        console.log('remove complete')
+        cb(err)
+      })
+    }))
+  }
+
+  // no top level lifecycles on rm
+  runPreinstallTopLevelLifecycles (cb) { cb() }
+  runPostinstallTopLevelLifecycles (cb) { cb() }
 }
 
-Uninstaller.prototype.runPreinstallTopLevelLifecycles = function (cb) { cb() }
-Uninstaller.prototype.runPostinstallTopLevelLifecycles = function (cb) { cb() }
+module.exports.Uninstaller = Uninstaller