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
14 changes: 8 additions & 6 deletions workspaces/arborist/lib/edge.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
const util = require('node:util')
const npa = require('npm-package-arg')
const depValid = require('./dep-valid.js')
const OverrideSet = require('./override-set.js')

class ArboristEdge {
constructor (edge) {
Expand Down Expand Up @@ -275,12 +274,15 @@ 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)) {
// Any inconsistency between the edge's override set and the target's override set is potentially problematic.
// But we only say the edge is in error if the override sets are plainly conflicting.
// Note that if the target doesn't have any dependencies of their own, then this inconsistency is irrelevant.
this.#error = 'INVALID'
} else {
// Note: We intentionally do NOT check for override conflicts here.
// Different override sets pointing to the same node are allowed as long as:
// 1. The node satisfies the version requirement (checked by satisfiedBy above)
// 2. Any actual version conflicts in the node's dependencies will be caught
// during normal dependency resolution in the build/reify phase
// The previous check (from b9225e524) was overly conservative and produced
// false positives, especially with reference overrides ($syntax) that resolve
// to the same effective versions despite being structurally different.
this.#error = 'OK'
}
}
Expand Down
46 changes: 37 additions & 9 deletions workspaces/arborist/test/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -3277,7 +3277,25 @@ t.test('should propagate the new override set to the target node', t => {
t.end()
})

t.test('should find inconsistency between the edge\'s override set and the target\'s override set', t => {
t.test('edges with different override contexts to same node should be valid', t => {
// Regression test for issue #8688
// Replaces the removed test 'should find inconsistency between the edge's override set
// and the target's override set' which was testing override conflict detection that
// was intentionally removed because it caused false positives.

// Create two different override sets (simulating Vaadin's structure)
const overridesComponents = new OverrideSet({
overrides: {
'@vaadin/react-components': '24.9.2',
},
})

const overridesComponentsPro = new OverrideSet({
overrides: {
'@vaadin/react-components-pro': '24.9.2',
},
})

const tree = new Node({
loadOverrides: true,
path: '/root',
Expand Down Expand Up @@ -3307,18 +3325,28 @@ t.test('should find inconsistency between the edge\'s override set and the targe
}],
})

// Force edge.override to a conflicting object so that it will differ from
// the computed override coming from the parent's override set.
const conflictingOverride = new OverrideSet({
overrides: { mockDep: '1.x' },
})
const edge = tree.edgesOut.get('mockDep')
edge.overrides = conflictingOverride

// Override satisfiedBy so it returns true, ensuring the conflict branch is reached
// Manually set an override to the edge
edge.overrides = overridesComponents

// Override satisfiedBy so it returns true, simulating that the node satisfies the version
edge.satisfiedBy = () => true

t.equal(tree.edgesOut.get('mockDep').error, 'INVALID', 'Edge should be marked INVALID due to conflicting overrides')
// Before the fix (b9225e524), if we then changed the target node's override to a different
// set, the edge would be marked INVALID due to override conflict detection.
// After the fix, the edge should remain valid because:
// 1. satisfiedBy returns true (version is satisfied)
// 2. We no longer check for override conflicts at the edge level
const mockDep = tree.children.get('mockDep')
mockDep.overrides = overridesComponentsPro

// Force edge to recalculate
edge.reload(true)

// The edge should be valid despite different override contexts
t.equal(edge.error, null, 'Edge should be valid despite different override contexts')
t.ok(edge.valid, 'Edge.valid should be true')

t.end()
})
Expand Down
Loading