Skip to content
This repository was archived by the owner on Aug 11, 2022. It is now read-only.

Commit bc263c3

Browse files
iarnaiarna
committed
install: Stop removing failing optionals from the tree
Credit: @iarna Reviewed-By: @zkat PR-URL: #19054 Fixes: #10335 Fixes: #2679 Fixes: #18380
1 parent cac2250 commit bc263c3

3 files changed

Lines changed: 33 additions & 56 deletions

File tree

‎lib/install/actions.js‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,23 +83,16 @@ function markAsFailed (pkg) {
8383
if (pkg.failed) return
8484
pkg.failed = true
8585
pkg.requires.forEach((req) => {
86-
req.requiredBy = req.requiredBy.filter((reqReqBy) => {
87-
return reqReqBy !== pkg
88-
})
89-
if (req.requiredBy.length === 0 && !req.userRequired) {
86+
var requiredBy = req.requiredBy.filter((reqReqBy) => !reqReqBy.failed)
87+
if (requiredBy.length === 0 && !req.userRequired) {
9088
markAsFailed(req)
9189
}
9290
})
9391
}
9492

9593
function handleOptionalDepErrors (pkg, err) {
9694
markAsFailed(pkg)
97-
var anyFatal = pkg.userRequired || pkg.isTop
98-
for (var ii = 0; ii < pkg.requiredBy.length; ++ii) {
99-
var parent = pkg.requiredBy[ii]
100-
var isFatal = failedDependency(parent, pkg)
101-
if (isFatal) anyFatal = true
102-
}
95+
var anyFatal = failedDependency(pkg)
10396
if (anyFatal) {
10497
throw err
10598
} else {

‎lib/install/deps.js‎

Lines changed: 24 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ function computeMetadata (tree, seen) {
122122
}
123123
}
124124

125-
tree.children.filter((child) => !child.removed && !child.failed).forEach((child) => computeMetadata(child, seen))
125+
tree.children.filter((child) => !child.removed).forEach((child) => computeMetadata(child, seen))
126126

127127
return tree
128128
}
@@ -277,7 +277,7 @@ function isNotEmpty (value) {
277277
return value != null && value !== ''
278278
}
279279

280-
module.exports.computeVersionSpec = computeVersionSpec
280+
exports.computeVersionSpec = computeVersionSpec
281281
function computeVersionSpec (tree, child) {
282282
validate('OO', arguments)
283283
var requested
@@ -309,10 +309,6 @@ function moduleNameMatches (name) {
309309
return function (child) { return moduleName(child) === name }
310310
}
311311

312-
function noModuleNameMatches (name) {
313-
return function (child) { return moduleName(child) !== name }
314-
}
315-
316312
// while this implementation does not require async calling, doing so
317313
// gives this a consistent interface with loadDeps et al
318314
exports.removeDeps = function (args, tree, saveToDependencies, next) {
@@ -378,19 +374,12 @@ function isDepOptional (tree, name, pkg) {
378374
return false
379375
}
380376

381-
var failedDependency = exports.failedDependency = function (tree, name_pkg) {
382-
var name
383-
var pkg = {}
384-
if (typeof name_pkg === 'string') {
385-
name = name_pkg
386-
} else {
387-
pkg = name_pkg
388-
name = moduleName(pkg)
389-
}
390-
tree.children = tree.children.filter(noModuleNameMatches(name))
391-
392-
if (isDepOptional(tree, name, pkg)) {
393-
return false
377+
exports.failedDependency = failedDependency
378+
function failedDependency (tree, name, pkg) {
379+
if (name) {
380+
if (isDepOptional(tree, name, pkg || {})) {
381+
return false
382+
}
394383
}
395384

396385
tree.failed = true
@@ -399,17 +388,16 @@ var failedDependency = exports.failedDependency = function (tree, name_pkg) {
399388

400389
if (tree.userRequired) return true
401390

402-
removeObsoleteDep(tree)
403-
404391
if (!tree.requiredBy) return false
405392

393+
let anyFailed = false
406394
for (var ii = 0; ii < tree.requiredBy.length; ++ii) {
407395
var requireParent = tree.requiredBy[ii]
408-
if (failedDependency(requireParent, tree.package)) {
409-
return true
396+
if (failedDependency(requireParent, moduleName(tree), tree)) {
397+
anyFailed = true
410398
}
411399
}
412-
return false
400+
return anyFailed
413401
}
414402

415403
function andHandleOptionalErrors (log, tree, name, done) {
@@ -419,7 +407,6 @@ function andHandleOptionalErrors (log, tree, name, done) {
419407
if (!er) return done(er, child, childLog)
420408
var isFatal = failedDependency(tree, name)
421409
if (er && !isFatal) {
422-
tree.children = tree.children.filter(noModuleNameMatches(name))
423410
reportOptionalFailure(tree, name, er)
424411
return done()
425412
} else {
@@ -602,8 +589,9 @@ function resolveWithNewModule (pkg, tree, log, next) {
602589
validate('OOOF', arguments)
603590

604591
log.silly('resolveWithNewModule', packageId(pkg), 'checking installable status')
605-
return isInstallable(pkg, iferr(next, function () {
606-
addBundled(pkg, iferr(next, function () {
592+
return isInstallable(pkg, (err) => {
593+
let installable = !err
594+
addBundled(pkg, (bundleErr) => {
607595
var parent = earliestInstallable(tree, tree, pkg) || tree
608596
var isLink = pkg._requested.type === 'directory'
609597
var child = createChild({
@@ -614,8 +602,9 @@ function resolveWithNewModule (pkg, tree, log, next) {
614602
children: pkg._bundled || [],
615603
isLink: isLink,
616604
isInLink: parent.isLink,
617-
knownInstallable: true
605+
knownInstallable: installable
618606
})
607+
if (!installable || bundleErr) child.failed = true
619608
delete pkg._bundled
620609
var hasBundled = child.children.length
621610

@@ -631,13 +620,14 @@ function resolveWithNewModule (pkg, tree, log, next) {
631620
}
632621

633622
if (pkg._shrinkwrap && pkg._shrinkwrap.dependencies) {
634-
return inflateShrinkwrap(child, pkg._shrinkwrap, function (er) {
635-
next(er, child, log)
623+
return inflateShrinkwrap(child, pkg._shrinkwrap, (swErr) => {
624+
if (swErr) child.failed = true
625+
next(err || bundleErr || swErr, child, log)
636626
})
637627
}
638-
next(null, child, log)
639-
}))
640-
}))
628+
next(err || bundleErr, child, log)
629+
})
630+
})
641631
}
642632

643633
var validatePeerDeps = exports.validatePeerDeps = function (tree, onInvalid) {
@@ -670,7 +660,7 @@ var findRequirement = exports.findRequirement = function (tree, name, requested,
670660
validate('OSO', [tree, name, requested])
671661
if (!requestor) requestor = tree
672662
var nameMatch = function (child) {
673-
return moduleName(child) === name && child.parent && !child.removed && !child.failed
663+
return moduleName(child) === name && child.parent && !child.removed
674664
}
675665
var versionMatch = function (child) {
676666
return doesChildVersionMatch(child, requested, requestor)

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

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ test('setup', function (t) {
158158
})
159159
})
160160
test('go go test racer', function (t) {
161-
common.npm(
161+
return common.npm(
162162
[
163163
'--prefix', pkg,
164164
'--fetch-retries', '0',
@@ -175,19 +175,13 @@ test('go go test racer', function (t) {
175175
PATH: process.env.PATH,
176176
Path: process.env.Path
177177
},
178-
stdio: [ 0, 'pipe', 2 ]
179-
},
180-
function (er, code, stdout, stderr) {
181-
t.ifError(er, 'install ran to completion without error')
182-
t.is(code, 0, 'npm install exited with code 0')
178+
stdio: 'pipe'
179+
}).spread((code, stdout, stderr) => {
180+
t.comment(stdout.trim())
183181
t.comment(stderr.trim())
184-
// stdout should be empty, because we only have one, optional, dep and
185-
// if it fails we shouldn't try installing anything
186-
t.equal(stdout, '')
182+
t.is(code, 0, 'npm install exited with code 0')
187183
t.notOk(/not ok/.test(stdout), 'should not contain the string \'not ok\'')
188-
t.end()
189-
}
190-
)
184+
})
191185
})
192186

193187
test('verify results', function (t) {

0 commit comments

Comments
 (0)