From aa9915301362eacd27bc7fa5a7d3d73a17895bbd Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 18 Feb 2021 14:58:55 -0800 Subject: [PATCH] Unpack shrinkwrapped deps not already unpacked We loop over the diff.leaves to find all the shrinkwraps that _must_ be unpacked ahead of time in order to complete the idealTree. However, if the idealTree already contains the children of the shrinkwrap-containing module (because it's been previously installed and saved to a lockfile), then we saw the hasShrinkwrap flag, and assumed it had already been unpacked. This tracks a Set of all nodes unpacked for the purposes of reading their shrinkwraps, and only skips _those_ modules at unpackNewModules. Different approach than #233, without adding extra items into the diff.leaves, which can have the side effect of calling mkdirp more than necessary. Close: #233 Fix: https://github.com/npm/cli/issues/2251 Reviewed-by: @nlf --- lib/arborist/reify.js | 9 ++++++-- .../test-arborist-reify.js-TAP.test.js | 8 +++---- test/arborist/reify.js | 21 ++++++++++++------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js index 7fd0728b0..9854d2d0b 100644 --- a/lib/arborist/reify.js +++ b/lib/arborist/reify.js @@ -44,6 +44,7 @@ const _loadTrees = Symbol.for('loadTrees') const _diffTrees = Symbol.for('diffTrees') const _createSparseTree = Symbol.for('createSparseTree') const _loadShrinkwrapsAndUpdateTrees = Symbol.for('loadShrinkwrapsAndUpdateTrees') +const _shrinkwrapUnpacked = Symbol('shrinkwrapUnpacked') const _reifyNode = Symbol.for('reifyNode') const _extractOrLink = Symbol('extractOrLink') // defined by rebuild mixin @@ -103,6 +104,7 @@ module.exports = cls => class Reifier extends cls { this.diff = null this[_retiredPaths] = {} + this[_shrinkwrapUnpacked] = new Set() this[_retiredUnchanged] = {} this[_sparseTreeDirs] = new Set() this[_sparseTreeRoots] = new Set() @@ -405,7 +407,8 @@ module.exports = cls => class Reifier extends cls { // shrinkwrap nodes define their dependency branches with a file, so // we need to unpack them, read that shrinkwrap file, and then update // the tree by calling loadVirtual with the node as the root. - [_loadShrinkwrapsAndUpdateTrees] (seen = new Set()) { + [_loadShrinkwrapsAndUpdateTrees] () { + const seen = this[_shrinkwrapUnpacked] const shrinkwraps = this.diff.leaves .filter(d => (d.action === 'CHANGE' || d.action === 'ADD') && d.ideal.hasShrinkwrap && !seen.has(d.ideal) && @@ -429,6 +432,8 @@ module.exports = cls => class Reifier extends cls { // reload the diff and sparse tree because the ideal tree changed .then(() => this[_diffTrees]()) .then(() => this[_createSparseTree]()) + .then(() => this[_addOmitsToTrashList]()) + .then(() => this[_loadShrinkwrapsAndUpdateTrees]()) .then(() => process.emit('timeEnd', 'reify:loadShrinkwraps')) } @@ -731,7 +736,7 @@ module.exports = cls => class Reifier extends cls { const node = diff.ideal const bd = node.package.bundleDependencies - const sw = node.hasShrinkwrap + const sw = this[_shrinkwrapUnpacked].has(node) // check whether we still need to unpack this one. // test the inDepBundle last, since that's potentially a tree walk. diff --git a/tap-snapshots/test-arborist-reify.js-TAP.test.js b/tap-snapshots/test-arborist-reify.js-TAP.test.js index 8e94a08c6..fc6ff1735 100644 --- a/tap-snapshots/test-arborist-reify.js-TAP.test.js +++ b/tap-snapshots/test-arborist-reify.js-TAP.test.js @@ -28686,7 +28686,7 @@ ArboristNode { } ` -exports[`test/arborist/reify.js TAP reifying with shronk warp dep shrinkwrapped-dep-no-lock > expect resolving Promise 1`] = ` +exports[`test/arborist/reify.js TAP reifying with shronk warp dep shrinkwrapped-dep-no-lock > must match snapshot 1`] = ` ArboristNode { "children": Map { "@isaacs/shrinkwrapped-dependency" => ArboristNode { @@ -28730,7 +28730,7 @@ ArboristNode { } ` -exports[`test/arborist/reify.js TAP reifying with shronk warp dep shrinkwrapped-dep-no-lock-empty > expect resolving Promise 1`] = ` +exports[`test/arborist/reify.js TAP reifying with shronk warp dep shrinkwrapped-dep-no-lock-empty > must match snapshot 1`] = ` ArboristNode { "children": Map { "@isaacs/shrinkwrapped-dependency" => ArboristNode { @@ -28790,7 +28790,7 @@ ArboristNode { } ` -exports[`test/arborist/reify.js TAP reifying with shronk warp dep shrinkwrapped-dep-with-lock > expect resolving Promise 1`] = ` +exports[`test/arborist/reify.js TAP reifying with shronk warp dep shrinkwrapped-dep-with-lock > must match snapshot 1`] = ` ArboristNode { "children": Map { "@isaacs/shrinkwrapped-dependency" => ArboristNode { @@ -28850,7 +28850,7 @@ ArboristNode { } ` -exports[`test/arborist/reify.js TAP reifying with shronk warp dep shrinkwrapped-dep-with-lock-empty > expect resolving Promise 1`] = ` +exports[`test/arborist/reify.js TAP reifying with shronk warp dep shrinkwrapped-dep-with-lock-empty > must match snapshot 1`] = ` ArboristNode { "children": Map { "@isaacs/shrinkwrapped-dependency" => ArboristNode { diff --git a/test/arborist/reify.js b/test/arborist/reify.js index f5ef05641..dc9210334 100644 --- a/test/arborist/reify.js +++ b/test/arborist/reify.js @@ -400,13 +400,20 @@ t.test('reifying with shronk warp dep', t => { 'shrinkwrapped-dep-no-lock-empty', ] t.plan(cases.length) - cases.forEach(c => t.test(c, t => - t.resolveMatchSnapshot(printReified(fixture(t, c), { - // set update so that we don't start the idealTree - // with the actualTree, and can see that the deps - // are indeed getting set up from the shrink wrap - update: /no-lock/.test(c), - })))) + for (const c of cases) { + t.test(c, async t => { + const path = fixture(t, c) + const tree = await printReified(path, { + // set update so that we don't start the idealTree + // with the actualTree, and can see that the deps + // are indeed getting set up from the shrink wrap + update: /no-lock/.test(c), + }) + t.matchSnapshot(tree) + const dep = `${path}/node_modules/@isaacs/shrinkwrapped-dependency` + t.equal(fs.statSync(`${dep}/package.json`).isFile(), true, 'has package.json') + }) + } }) t.test('link deps already in place', t =>