From 2ba09cc0d7d56a064aa67bbb1881d381e6504888 Mon Sep 17 00:00:00 2001 From: nlf Date: Mon, 7 Feb 2022 09:35:36 -0800 Subject: [PATCH] fix(arborist): check if a spec is a workspace before fetching a manifest, closes #3637 (#4371) --- .../arborist/lib/arborist/build-ideal-tree.js | 50 ++++++---- .../test/arborist/build-ideal-tree.js | 99 +++++++++++++++++++ 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 0375e1851495a..b7bc56f3e9797 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 368df05bcfaf4..e718b0b58cca4 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() })