fix(arborist): check if a spec is a workspace before fetching a manifest, closes #3637 (#4371)

2 files changed, 132 insertions, 17 deletions

diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js
index 0375e1851..b7bc56f3e 100644
--- a/workspaces/arborist/lib/arborist/build-ideal-tree.js
+++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js
@@ -1250,24 +1250,40 @@ This is a one-time fix-up, please be patient...
     // Don't bother to load the manifest for link deps, because the target
     // might be within another package that doesn't exist yet.
     const { legacyPeerDeps } = this
-    return spec.type === 'directory'
-      ? this[_linkFromSpec](name, spec, parent, edge)
-      : this[_fetchManifest](spec)
-        .then(pkg => new Node({ name, pkg, parent, legacyPeerDeps }), error => {
-          error.requiredBy = edge.from.location || '.'
-
-          // failed to load the spec, either because of enotarget or
-          // fetch failure of some other sort.  save it so we can verify
-          // later that it's optional, otherwise the error is fatal.
-          const n = new Node({
-            name,
-            parent,
-            error,
-            legacyPeerDeps,
-          })
-          this[_loadFailures].add(n)
-          return n
+
+    // spec is a directory, link it
+    if (spec.type === 'directory') {
+      return this[_linkFromSpec](name, spec, parent, edge)
+    }
+
+    // if the spec matches a workspace name, then see if the workspace node will
+    // satisfy the edge. if it does, we return the workspace node to make sure it
+    // takes priority.
+    if (this.idealTree.workspaces && this.idealTree.workspaces.has(spec.name)) {
+      const existingNode = this.idealTree.edgesOut.get(spec.name).to
+      if (existingNode && existingNode.isWorkspace && existingNode.satisfies(edge)) {
+        return edge.to
+      }
+    }
+
+    // spec isn't a directory, and either isn't a workspace or the workspace we have
+    // doesn't satisfy the edge. try to fetch a manifest and build a node from that.
+    return this[_fetchManifest](spec)
+      .then(pkg => new Node({ name, pkg, parent, legacyPeerDeps }), error => {
+        error.requiredBy = edge.from.location || '.'
+
+        // failed to load the spec, either because of enotarget or
+        // fetch failure of some other sort.  save it so we can verify
+        // later that it's optional, otherwise the error is fatal.
+        const n = new Node({
+          name,
+          parent,
+          error,
+          legacyPeerDeps,
         })
+        this[_loadFailures].add(n)
+        return n
+      })
   }
 
   [_linkFromSpec] (name, spec, parent, edge) {
diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js
index 368df05bc..e718b0b58 100644
--- a/workspaces/arborist/test/arborist/build-ideal-tree.js
+++ b/workspaces/arborist/test/arborist/build-ideal-tree.js
@@ -877,6 +877,105 @@ t.test('workspaces', t => {
     t.matchSnapshot(printTree(await tree))
   })
 
+  t.test('workspace nodes are used instead of fetching manifests when they are valid', async t => {
+    // turn off networking, this should never make a registry request
+    nock.disableNetConnect()
+    t.teardown(() => nock.enableNetConnect())
+
+    const path = t.testdir({
+      'package.json': JSON.stringify({
+        name: 'root',
+        workspaces: ['workspace-a', 'workspace-b'],
+      }),
+      // the package-lock.json references version 1.0.0 of the workspace deps
+      // as it would if a user hand edited their workspace's package.json and
+      // now are attempting to reify with a stale package-lock
+      'package-lock.json': JSON.stringify({
+        name: 'root',
+        lockfileVersion: 2,
+        requires: true,
+        packages: {
+          '': {
+            name: 'root',
+            workspaces: ['workspace-a', 'workspace-b'],
+          },
+          'node_modules/workspace-a': {
+            resolved: 'workspace-a',
+            link: true,
+          },
+          'node_modules/workspace-b': {
+            resolved: 'workspace-b',
+            link: true,
+          },
+          'workspace-a': {
+            name: 'workspace-a',
+            version: '1.0.0',
+            dependencies: {
+              'workspace-b': '1.0.0',
+            },
+          },
+          'workspace-b': {
+            name: 'workspace-b',
+            version: '1.0.0',
+          },
+        },
+        dependencies: {
+          'workspace-a': {
+            version: 'file:workspace-a',
+            requires: {
+              'workspace-b': '1.0.0',
+            },
+          },
+          'workspace-b': {
+            version: 'file:workspace-b',
+          },
+        },
+      }),
+      node_modules: {
+        'workspace-a': t.fixture('symlink', '../workspace-a'),
+        'workspace-b': t.fixture('symlink', '../workspace-b'),
+      },
+      // the workspaces themselves are at 2.0.0 because they're what was edited
+      'workspace-a': {
+        'package.json': JSON.stringify({
+          name: 'workspace-a',
+          version: '2.0.0',
+          dependencies: {
+            'workspace-b': '2.0.0',
+          },
+        }),
+      },
+      'workspace-b': {
+        'package.json': JSON.stringify({
+          name: 'workspace-b',
+          version: '2.0.0',
+        }),
+      },
+    })
+
+    const arb = new Arborist({
+      ...OPT,
+      path,
+      workspaces: ['workspace-a', 'workspace-b'],
+    })
+
+    // this will reject if we try to fetch a manifest for some reason
+    const tree = await arb.buildIdealTree({
+      path,
+    })
+
+    const edgeA = tree.edgesOut.get('workspace-a')
+    t.ok(edgeA.valid, 'workspace-a should be valid')
+    const edgeB = tree.edgesOut.get('workspace-b')
+    t.ok(edgeB.valid, 'workspace-b should be valid')
+    const nodeA = edgeA.to.target
+    t.ok(nodeA.isWorkspace, 'workspace-a is definitely a workspace')
+    const nodeB = edgeB.to.target
+    t.ok(nodeB.isWorkspace, 'workspace-b is definitely a workspace')
+    const nodeBfromA = nodeA.edgesOut.get('workspace-b').to.target
+    t.equal(nodeBfromA, nodeB, 'workspace-b edgeOut from workspace-a is the workspace')
+  })
+
   t.end()
 })