-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: issue-5850 - Settle override conflicts between edges and propagate changes #7025
base: latest
Are you sure you want to change the base?
Changes from 10 commits
865070c
cd20196
0cad4e0
6b276f3
fff3072
e97176e
19155ed
dedbccf
4a83786
7ee2f2e
0917421
6d0ad42
67d7d5c
643cbea
c5f6e4a
6c9bbf9
63760d9
6111630
b42d4c8
7d97726
35e5790
35ce2f7
5630954
f859e92
29f0750
17ec457
4cde831
701b125
b119f67
3136b64
80126c7
c4b5338
9db1232
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1344,9 +1344,100 @@ class Node { | |
this.edgesOut.set(edge.name, edge) | ||
} | ||
|
||
addEdgeIn (edge) { | ||
recalculateOutEdgesOverrides () { | ||
// For each edge out propogate the new overrides through. | ||
for (const [, edge] of this.edgesOut) { | ||
edge.reload(true) | ||
if (edge.to) { | ||
edge.to.updateNodeOverrideSet(edge.overrides) | ||
} | ||
} | ||
} | ||
|
||
findSpecificOverrideSet (first, second) { | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
for (let overrideSet = second; overrideSet; overrideSet = overrideSet.parent) { | ||
if (overrideSet.isEqual(first)) { | ||
return second | ||
} | ||
} | ||
for (let overrideSet = first; overrideSet; overrideSet = overrideSet.parent) { | ||
if (overrideSet.isEqual(second)) { | ||
return first | ||
} | ||
} | ||
console.log('Conflicting override sets') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wraithgar This case means that the same node has two edges with completely different override sets pointing to it. This log is useful for debugging, not sure if just to remove the line or there's some other convention used in the arborist. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
updateNodeOverrideSetDueToEdgeRemoval (otherOverrideSet) { | ||
// If this edge's overrides isn't equal to this node's overrides, then removing it won't change newOverrideSet later. | ||
if (!this.overrides || !this.overrides.isEqual(otherOverrideSet)) { | ||
return false | ||
} | ||
let newOverrideSet | ||
for (const edge of this.edgesIn) { | ||
if (newOverrideSet) { | ||
newOverrideSet = this.findSpecificOverrideSet(edge.overrides, newOverrideSet) | ||
} else { | ||
newOverrideSet = edge.overrides | ||
} | ||
} | ||
if (this.overrides.isEqual(newOverrideSet)) { | ||
return false | ||
} | ||
this.overrides = newOverrideSet | ||
if (this.overrides) { | ||
// Optimization: if there's any override set at all, then no non-extraneous node has an empty override set. So if we temporarily have no | ||
// override set (for example, we removed all the edges in), there's no use updating all the edges out right now. Let's just wait until | ||
// we have an actual override set later. | ||
this.recalculateOutEdgesOverrides() | ||
} | ||
return true | ||
} | ||
|
||
// This logic isn't perfect either. When we have two edges in that have different override sets, then we have to decide which set is correct. | ||
// This function assumes the more specific override set is applicable, so if we have dependencies A->B->C and A->C | ||
// and an override set that specifies what happens for C under A->B, this will work even if the new A->C edge comes along and tries to change | ||
// the override set. | ||
// The strictly correct logic is not to allow two edges with different overrides to point to the same node, because even if this node can satisfy | ||
// both, one of its dependencies might need to be different depending on the edge leading to it. | ||
// However, this might cause a lot of duplication, because the conflict in the dependencies might never actually happen. | ||
updateNodeOverrideSet (otherOverrideSet) { | ||
if (!this.overrides) { | ||
// Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree. | ||
// So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field. | ||
if (!otherOverrideSet) { | ||
return false | ||
} | ||
this.overrides = otherOverrideSet | ||
this.recalculateOutEdgesOverrides() | ||
return true | ||
} | ||
if (this.overrides.isEqual(otherOverrideSet)) { | ||
return false | ||
} | ||
const newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet) | ||
if (newOverrideSet) { | ||
if (!this.overrides.isEqual(newOverrideSet)) { | ||
this.overrides = newOverrideSet | ||
this.recalculateOutEdgesOverrides() | ||
return true | ||
} | ||
return false | ||
} | ||
// This is an error condition. We can only get here if the new override set is in conflict with the existing. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wraithgar This case means that the same node has two edges with completely different override sets pointing to it, which means that this node can't really decide which one is correct.
Currently I chose (1), but maybe it should at least show some kind of warning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A hard-won lesson we learned with peer dependencies is that
So, unless there's a really elegant way to do This also leaves the door open later to implementing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @wraithgar, I've encountered a problem with throwing an exception here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Until clarity on the issue you can always There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I checked the behavior of the algorithms here. @jdalton @wraithgar |
||
} | ||
|
||
deleteEdgeIn (edge) { | ||
this.edgesIn.delete(edge) | ||
if (edge.overrides) { | ||
this.overrides = edge.overrides | ||
this.updateNodeOverrideSetDueToEdgeRemoval(edge.overrides) | ||
} | ||
} | ||
|
||
addEdgeIn (edge) { | ||
// We need to handle the case where the new edge in has an overrides field which is different from the current value. | ||
if (!this.overrides || !this.overrides.isEqual(edge.overrides)) { | ||
this.updateNodeOverrideSet(edge.overrides) | ||
} | ||
|
||
this.edgesIn.add(edge) | ||
|
This comment was marked as resolved.
Sorry, something went wrong.