Skip to content
Open
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
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/edge.js
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ class Edge {
this.#error = 'PEER LOCAL'
} else if (!this.satisfiedBy(this.#to)) {
this.#error = 'INVALID'
} else if (this.overrides && this.#to.edgesOut.size && OverrideSet.doOverrideSetsConflict(this.overrides, this.#to.overrides)) {
} else if (this.overrides && this.#to.overrides && this.#to.edgesOut.size && OverrideSet.doOverrideSetsConflict(this.overrides, this.#to.overrides)) {
// Check for conflicts between the edge's override set and the target node's override set.
// This catches cases where different parts of the tree have genuinely incompatible
// version requirements for the same package.
Expand Down
5 changes: 5 additions & 0 deletions workspaces/arborist/lib/override-set.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,11 @@ class OverrideSet {
}

static doOverrideSetsConflict (first, second) {
// If either override set is missing, there's no conflict
if (!first || !second) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're only gonna be able to get coverage here with a unit test because half of this is already being done upstream

else if (this.overrides && this.#to.edgesOut.size && OverrideSet.doOverrideSetsConflict(this.overrides, this.#to.overrides)) 

So, we should add && this.#to.overrides to the calling code intead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OR remove the this.overrides from the calling code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wraithgar done - i added the this.#to.overrides to the callsite. I kept the original guard as well because I think it is still practical and safe.

return false
}

// If override sets contain one another then we can try to use the more specific one.
// If neither one is more specific, check for semantic conflicts.
const specificSet = this.findSpecificOverrideSet(first, second)
Expand Down
88 changes: 88 additions & 0 deletions workspaces/arborist/test/edge.js
Original file line number Diff line number Diff line change
Expand Up @@ -1244,3 +1244,91 @@ t.test('override fallback to local when root missing dependency with from.overri
t.equal(edge.spec, '^1.2.3', 'should fall back to local package version from devDependencies')
t.end()
})

t.test('edge with overrides should not crash when target has no overrides', t => {
const root = {
name: 'root',
edgesOut: new Map(),
edgesIn: new Set(),
explain: () => 'root',
package: {
name: 'root',
version: '1.0.0',
overrides: {
react: '^18.3.1',
'react-dom': '^18.3.1',
},
},
get version () {
return this.package.version
},
isTop: true,
parent: null,
overrides: new OverrideSet({
overrides: {
react: '^18.3.1',
'react-dom': '^18.3.1',
},
}),
resolve (n) {
return n === 'dep' ? dep : null
},
addEdgeOut (edge) {
this.edgesOut.set(edge.name, edge)
},
addEdgeIn (edge) {
this.edgesIn.add(edge)
},
deleteEdgeIn (edge) {
this.edgesIn.delete(edge)
},
}

// Target node with dependencies but NO overrides
const dep = {
name: 'dep',
edgesOut: new Map([
// Has dependencies so edgesOut.size > 0
['some-lib', { name: 'some-lib', peer: false }],
]),
edgesIn: new Set(),
explain: () => 'dep',
package: { name: 'dep', version: '1.0.0' },
get version () {
return this.package.version
},
parent: root,
root,
// NO overrides property - this causes this.#to.overrides to be undefined
resolve () {
return null
},
addEdgeOut (edge) {
this.edgesOut.set(edge.name, edge)
},
addEdgeIn (edge) {
this.edgesIn.add(edge)
},
deleteEdgeIn (edge) {
this.edgesIn.delete(edge)
},
}

// Create an edge from root to dep
// Root has overrides, dep has dependencies but no overrides
const edge = new Edge({
from: root,
type: 'prod',
spec: '^1.0.0',
name: 'dep',
overrides: root.overrides,
})

t.doesNotThrow(() => {
const err = edge.error
t.equal(err, null, 'edge should be valid (no error)')
}, 'should not crash when target node has no overrides')

t.ok(edge.valid, 'edge should be valid')
t.end()
})
20 changes: 20 additions & 0 deletions workspaces/arborist/test/override-set.js
Original file line number Diff line number Diff line change
Expand Up @@ -558,4 +558,24 @@ t.test('coverage for isEqual edge cases', async t => {
// AND parent.isEqual(...) returns false
t.notOk(childC.isEqual(childD), 'two children with different parents are not equal')
})

t.test('should handle undefined/null second parameter gracefully', async (t) => {
const overrides = new OverrideSet({
overrides: {
react: '^18.3.1',
'react-dom': '^18.3.1',
},
})

t.doesNotThrow(() => {
OverrideSet.doOverrideSetsConflict(overrides, undefined)
}, 'should not throw when second parameter is undefined')

t.doesNotThrow(() => {
OverrideSet.doOverrideSetsConflict(overrides, null)
}, 'should not throw when second parameter is null')

t.equal(OverrideSet.doOverrideSetsConflict(overrides, undefined), false, 'should return false for undefined')
t.equal(OverrideSet.doOverrideSetsConflict(overrides, null), false, 'should return false for null')
})
})
Loading