From 4d676e31a68f081b8553eff4e79db1f29acf47e1 Mon Sep 17 00:00:00 2001 From: nlf Date: Thu, 7 Apr 2022 14:37:51 -0700 Subject: [PATCH] fix(arborist): when reloading an edge, also refresh overrides this fixes some scenarios where overrides were not being properly applied, especially those where a node_modules or package-lock.json already exists --- workspaces/arborist/lib/edge.js | 5 +++ workspaces/arborist/lib/node.js | 3 ++ workspaces/arborist/test/edge.js | 62 +++++++++++++++++++------------- workspaces/arborist/test/node.js | 56 +++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 25 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 87439e7645366..d72f312569466 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -215,6 +215,11 @@ class Edge { reload (hard = false) { this[_explanation] = null + if (this[_from].overrides) { + this.overrides = this[_from].overrides.getEdgeRule(this) + } else { + delete this.overrides + } const newTo = this[_from].resolve(this.name) if (newTo !== this[_to]) { if (this[_to]) { diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 45c288bcf6cf7..c79bc0bd3a00b 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -792,6 +792,9 @@ class Node { target.root = root } + if (!this.overrides && this.parent && this.parent.overrides) { + this.overrides = this.parent.overrides.getNodeRule(this) + } // tree should always be valid upon root setter completion. treeCheck(this) treeCheck(root) diff --git a/workspaces/arborist/test/edge.js b/workspaces/arborist/test/edge.js index c90b202786d0d..2df989eb82f89 100644 --- a/workspaces/arborist/test/edge.js +++ b/workspaces/arborist/test/edge.js @@ -244,16 +244,19 @@ t.not(new Edge({ }).satisfiedBy(b), 'b does not satisfy edge for c') reset(a) +const overrideSet = new OverrideSet({ + overrides: { + c: '2.x', + }, +}) + +a.overrides = overrideSet t.matchSnapshot(new Edge({ from: a, type: 'prod', name: 'c', spec: '1.x', - overrides: new OverrideSet({ - overrides: { - c: '2.x', - }, - }).getEdgeRule({ name: 'c', spec: '1.x' }), + overrides: overrideSet.getEdgeRule({ name: 'c', spec: '1.x' }), }).toJSON(), 'printableEdge shows overrides') reset(a) @@ -262,11 +265,7 @@ const overriddenExplanation = new Edge({ type: 'prod', name: 'c', spec: '1.x', - overrides: new OverrideSet({ - overrides: { - c: '2.x', - }, - }).getEdgeRule({ name: 'c', spec: '1.x' }), + overrides: overrideSet.getEdgeRule({ name: 'c', spec: '1.x' }), }).explain() t.equal(overriddenExplanation.rawSpec, '1.x', 'rawSpec has original spec') t.equal(overriddenExplanation.spec, '2.x', 'spec has override spec') @@ -278,24 +277,22 @@ t.ok(new Edge({ type: 'prod', name: 'c', spec: '1.x', - overrides: new OverrideSet({ - overrides: { - c: '2.x', - }, - }).getEdgeRule({ name: 'c', spec: '1.x' }), + overrides: overrideSet.getEdgeRule({ name: 'c', spec: '1.x' }), }).satisfiedBy(c), 'c@2 satisfies spec:1.x, override:2.x') reset(a) +const overrideSetB = new OverrideSet({ + overrides: { + b: '1.x', + }, +}) +a.overrides = overrideSetB t.matchSnapshot(new Edge({ from: a, type: 'prod', name: 'c', spec: '2.x', - overrides: new OverrideSet({ - overrides: { - b: '1.x', - }, - }).getEdgeRule({ name: 'c', spec: '2.x' }), + overrides: overrideSetB.getEdgeRule({ name: 'c', spec: '2.x' }), }).toJSON(), 'printableEdge does not show non-applicable override') t.ok(new Edge({ @@ -303,13 +300,26 @@ t.ok(new Edge({ type: 'prod', name: 'c', spec: '2.x', - overrides: new OverrideSet({ - overrides: { - b: '1.x', - }, - }).getEdgeRule({ name: 'c', spec: '2.x' }), + overrides: overrideSetB.getEdgeRule({ name: 'c', spec: '2.x' }), }).satisfiedBy(c), 'c@2 satisfies spec:1.x, no matching override') reset(a) +delete a.overrides + +const overrideEdge = new Edge({ + from: a, + type: 'prod', + name: 'c', + spec: '2.x', +}) +t.notOk(overrideEdge.overrides, 'edge has no overrides') +a.overrides = new OverrideSet({ overrides: { b: '1.x' } }) +t.notOk(overrideEdge.overrides, 'edge has no overrides') +overrideEdge.reload() +t.ok(overrideEdge.overrides, 'edge has overrides after reload') +delete a.overrides +overrideEdge.reload() +t.notOk(overrideEdge.overrides, 'edge has no overrides after reload') +reset(a) const referenceTop = { name: 'referenceTop', @@ -494,6 +504,7 @@ const overrides = new OverrideSet({ c: '1.x', }, }) +a.overrides = overrides const overriddenEdge = new Edge({ from: a, type: 'prod', @@ -504,6 +515,7 @@ const overriddenEdge = new Edge({ t.equal(overriddenEdge.spec, '1.x', 'override spec takes priority') t.equal(overriddenEdge.rawSpec, '2.x', 'rawSpec holds original spec') reset(a) +delete a.overrides const old = new Edge({ from: a, diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index ecdf72c22a6dc..80bc21559569b 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -2781,6 +2781,62 @@ t.test('overrides', (t) => { t.end() }) + t.test('setting root replaces overrides', async (t) => { + const root = new Node({ + path: '/some/path', + loadOverrides: true, + pkg: { + name: 'root', + version: '1.0.0', + dependencies: { + foo: '^1.0.0', + }, + overrides: { + bar: '^2.0.0', + }, + }, + }) + + const foo = new Node({ + path: '/some/path/node_modules/foo', + pkg: { + name: 'foo', + version: '1.0.0', + dependencies: { + bar: '^1.0.0', + }, + }, + }) + + const bar = new Node({ + path: '/some/path/node_modules/bar', + pkg: { + name: 'bar', + version: '2.0.0', + }, + }) + + t.ok(root.overrides, 'root has overrides') + t.notOk(foo.overrides, 'foo does not have overrides') + t.notOk(bar.overrides, 'bar does not have overrides') + t.notOk(root.edgesOut.get('foo').valid, 'foo edge is not valid') + t.notOk(foo.edgesOut.get('bar').valid, 'bar edge is not valid') + + // we add bar to the root first, this is deliberate so that we don't have a simple + // linear inheritance. we'll add foo later and make sure that both edges and nodes + // become valid after that + + bar.root = root + t.ok(bar.overrides, 'bar now has overrides') + t.notOk(foo.edgesOut.get('bar').valid, 'bar edge is not valid yet') + + foo.root = root + t.ok(foo.overrides, 'foo now has overrides') + t.ok(root.edgesOut.get('foo').valid, 'foo edge is now valid') + t.ok(bar.overrides, 'bar still has overrides') + t.ok(foo.edgesOut.get('bar').valid, 'bar edge is now valid') + }) + t.test('canReplaceWith requires the same overrides', async (t) => { const original = new Node({ loadOverrides: true,