Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(arborist): when reloading an edge, also refresh overrides #4709

Merged
merged 2 commits into from
Apr 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions workspaces/arborist/lib/edge.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]) {
Expand Down
3 changes: 3 additions & 0 deletions workspaces/arborist/lib/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
16 changes: 8 additions & 8 deletions workspaces/arborist/tap-snapshots/test/printable.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -564,24 +564,24 @@ version:'1.0.0',
location:'',
path:'/some/path',
isProjectRoot:true,
overrides:Map{'foo@1' => '2.0.0', 'bar' => '2.0.0'},
edgesOut:Map{
'bar' =>{prod bar@^1.0.0 overridden:2.0.0 -> node_modules/bar},
'foo' =>{prod foo@^1.0.0 overridden:2.0.0 -> node_modules/foo}},
overrides:Map{'bar' => '2.0.0'},
edgesOut:Map{'foo' =>{prod foo@^1.0.0 -> node_modules/foo}},
children:Map{
'bar' =>{
name:'bar',
version:'2.0.0',
location:'node_modules/bar',
path:'/some/path/node_modules/bar',
overrides:Map{'bar' => '2.0.0', 'foo@1' => '2.0.0'},
edgesIn:Set{{"" prod bar@^1.0.0}}},
overrides:Map{'bar' => '2.0.0'},
edgesIn:Set{{node_modules/foo prod bar@^1.0.0}}},
'foo' =>{
name:'foo',
version:'2.0.0',
version:'1.0.0',
location:'node_modules/foo',
path:'/some/path/node_modules/foo',
overrides:Map{'foo@1' => '2.0.0', 'bar' => '2.0.0'},
overrides:Map{'bar' => '2.0.0'},
edgesOut:Map{
'bar' =>{prod bar@^1.0.0 overridden:2.0.0 -> node_modules/bar}},
edgesIn:Set{{"" prod foo@^1.0.0}}}}}
`

Expand Down
62 changes: 37 additions & 25 deletions workspaces/arborist/test/edge.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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')
Expand All @@ -278,38 +277,49 @@ 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({
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' }),
}).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',
Expand Down Expand Up @@ -494,6 +504,7 @@ const overrides = new OverrideSet({
c: '1.x',
},
})
a.overrides = overrides
const overriddenEdge = new Edge({
from: a,
type: 'prod',
Expand All @@ -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,
Expand Down
56 changes: 56 additions & 0 deletions workspaces/arborist/test/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 1 addition & 3 deletions workspaces/arborist/test/printable.js
Original file line number Diff line number Diff line change
Expand Up @@ -421,15 +421,13 @@ t.test('show overrides', (t) => {
version: '1.0.0',
dependencies: {
foo: '^1.0.0',
bar: '^1.0.0',
},
overrides: {
'foo@1': '2.0.0',
bar: '2.0.0',
},
},
children: [
{ pkg: { name: 'foo', version: '2.0.0' }, ...flags },
{ pkg: { name: 'foo', version: '1.0.0', dependencies: { bar: '^1.0.0' } }, ...flags },
{ pkg: { name: 'bar', version: '2.0.0' }, ...flags },
],
...flags,
Expand Down