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: npm/cli#2251
Reviewed-by: @nlf

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.

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 {

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 =>