From 6b5c6c9cc898e8459efe6ba72bf19b9790bf5d9a Mon Sep 17 00:00:00 2001
From: nlf <quitlahok@gmail.com>
Date: Thu, 3 Feb 2022 14:13:55 -0800
Subject: [PATCH] fix(arborist): check if a spec is a workspace before fetching
a manifest, closes #3637
---
.../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()
})