install: Stop removing failing optionals from the tree

Credit: @iarna
Reviewed-By: @zkat
PR-URL: #19054
Fixes: #10335
Fixes: #2679
Fixes: #18380

lib/install/actions.js

@@ -83,23 +83,16 @@ function markAsFailed (pkg) {
   if (pkg.failed) return
   pkg.failed = true
   pkg.requires.forEach((req) => {
-    req.requiredBy = req.requiredBy.filter((reqReqBy) => {
-      return reqReqBy !== pkg
-    })
-    if (req.requiredBy.length === 0 && !req.userRequired) {
+    var requiredBy = req.requiredBy.filter((reqReqBy) => !reqReqBy.failed)
+    if (requiredBy.length === 0 && !req.userRequired) {
       markAsFailed(req)
     }
   })
 }
 
 function handleOptionalDepErrors (pkg, err) {
   markAsFailed(pkg)
-  var anyFatal = pkg.userRequired || pkg.isTop
-  for (var ii = 0; ii < pkg.requiredBy.length; ++ii) {
-    var parent = pkg.requiredBy[ii]
-    var isFatal = failedDependency(parent, pkg)
-    if (isFatal) anyFatal = true
-  }
+  var anyFatal = failedDependency(pkg)
   if (anyFatal) {
     throw err
   } else {

lib/install/deps.js

@@ -122,7 +122,7 @@ function computeMetadata (tree, seen) {
     }
   }
 
-  tree.children.filter((child) => !child.removed && !child.failed).forEach((child) => computeMetadata(child, seen))
+  tree.children.filter((child) => !child.removed).forEach((child) => computeMetadata(child, seen))
 
   return tree
 }
@@ -277,7 +277,7 @@ function isNotEmpty (value) {
   return value != null && value !== ''
 }
 
-module.exports.computeVersionSpec = computeVersionSpec
+exports.computeVersionSpec = computeVersionSpec
 function computeVersionSpec (tree, child) {
   validate('OO', arguments)
   var requested
@@ -309,10 +309,6 @@ function moduleNameMatches (name) {
   return function (child) { return moduleName(child) === name }
 }
 
-function noModuleNameMatches (name) {
-  return function (child) { return moduleName(child) !== 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, next) {
@@ -378,19 +374,12 @@ function isDepOptional (tree, name, pkg) {
   return false
 }
 
-var failedDependency = exports.failedDependency = function (tree, name_pkg) {
-  var name
-  var pkg = {}
-  if (typeof name_pkg === 'string') {
-    name = name_pkg
-  } else {
-    pkg = name_pkg
-    name = moduleName(pkg)
-  }
-  tree.children = tree.children.filter(noModuleNameMatches(name))
-
-  if (isDepOptional(tree, name, pkg)) {
-    return false
+exports.failedDependency = failedDependency
+function failedDependency (tree, name, pkg) {
+  if (name) {
+    if (isDepOptional(tree, name, pkg || {})) {
+      return false
+    }
   }
 
   tree.failed = true
@@ -399,17 +388,16 @@ var failedDependency = exports.failedDependency = function (tree, name_pkg) {
 
   if (tree.userRequired) return true
 
-  removeObsoleteDep(tree)
-
   if (!tree.requiredBy) return false
 
+  let anyFailed = false
   for (var ii = 0; ii < tree.requiredBy.length; ++ii) {
     var requireParent = tree.requiredBy[ii]
-    if (failedDependency(requireParent, tree.package)) {
-      return true
+    if (failedDependency(requireParent, moduleName(tree), tree)) {
+      anyFailed = true
     }
   }
-  return false
+  return anyFailed
 }
 
 function andHandleOptionalErrors (log, tree, name, done) {
@@ -419,7 +407,6 @@ function andHandleOptionalErrors (log, tree, name, done) {
     if (!er) return done(er, child, childLog)
     var isFatal = failedDependency(tree, name)
     if (er && !isFatal) {
-      tree.children = tree.children.filter(noModuleNameMatches(name))
       reportOptionalFailure(tree, name, er)
       return done()
     } else {
@@ -602,8 +589,9 @@ function resolveWithNewModule (pkg, tree, log, next) {
   validate('OOOF', arguments)
 
   log.silly('resolveWithNewModule', packageId(pkg), 'checking installable status')
-  return isInstallable(pkg, iferr(next, function () {
-    addBundled(pkg, iferr(next, function () {
+  return isInstallable(pkg, (err) => {
+    let installable = !err
+    addBundled(pkg, (bundleErr) => {
       var parent = earliestInstallable(tree, tree, pkg) || tree
       var isLink = pkg._requested.type === 'directory'
       var child = createChild({
@@ -614,8 +602,9 @@ function resolveWithNewModule (pkg, tree, log, next) {
         children: pkg._bundled || [],
         isLink: isLink,
         isInLink: parent.isLink,
-        knownInstallable: true
+        knownInstallable: installable
       })
+      if (!installable || bundleErr) child.failed = true
       delete pkg._bundled
       var hasBundled = child.children.length
 
@@ -631,13 +620,14 @@ function resolveWithNewModule (pkg, tree, log, next) {
       }
 
       if (pkg._shrinkwrap && pkg._shrinkwrap.dependencies) {
-        return inflateShrinkwrap(child, pkg._shrinkwrap, function (er) {
-          next(er, child, log)
+        return inflateShrinkwrap(child, pkg._shrinkwrap, (swErr) => {
+          if (swErr) child.failed = true
+          next(err || bundleErr || swErr, child, log)
         })
       }
-      next(null, child, log)
-    }))
-  }))
+      next(err || bundleErr, child, log)
+    })
+  })
 }
 
 var validatePeerDeps = exports.validatePeerDeps = function (tree, onInvalid) {
@@ -670,7 +660,7 @@ var findRequirement = exports.findRequirement = function (tree, name, requested,
   validate('OSO', [tree, name, requested])
   if (!requestor) requestor = tree
   var nameMatch = function (child) {
-    return moduleName(child) === name && child.parent && !child.removed && !child.failed
+    return moduleName(child) === name && child.parent && !child.removed
   }
   var versionMatch = function (child) {
     return doesChildVersionMatch(child, requested, requestor)

test/tap/optional-metadep-rollback-collision.js

@@ -158,7 +158,7 @@ test('setup', function (t) {
   })
 })
 test('go go test racer', function (t) {
-  common.npm(
+  return common.npm(
     [
       '--prefix', pkg,
       '--fetch-retries', '0',
@@ -175,19 +175,13 @@ test('go go test racer', function (t) {
         PATH: process.env.PATH,
         Path: process.env.Path
       },
-      stdio: [ 0, 'pipe', 2 ]
-    },
-    function (er, code, stdout, stderr) {
-      t.ifError(er, 'install ran to completion without error')
-      t.is(code, 0, 'npm install exited with code 0')
+      stdio: 'pipe'
+    }).spread((code, stdout, stderr) => {
+      t.comment(stdout.trim())
       t.comment(stderr.trim())
-      // stdout should be empty, because we only have one, optional, dep and
-      // if it fails we shouldn't try installing anything
-      t.equal(stdout, '')
+      t.is(code, 0, 'npm install exited with code 0')
       t.notOk(/not ok/.test(stdout), 'should not contain the string \'not ok\'')
-      t.end()
-    }
-  )
+    })
 })
 
 test('verify results', function (t) {