fix: issue-5850 - Settle override conflicts between edges and propagate changes

[AlonNavon]

A first step in fixing the overrides feature. In this PR I'm aiming to fix 3 bugs:

1. When we add an edge going into a node we update the node's overrides, but we don't update the overrides of that node's outgoing edges, and so forth. We need the up-to-date overrides to filter through.
2. When we remove an edge going into a node we don't update the overrides at all (and we don't update the outgoing edges like in the previous bug).
3. When we add an edge going in, and we already have a different override set for the node, we just ignore the existing override set and overwrite it with that of the new edge. Instead, this PR chooses the most specific override set. This still isn't the absolutely correct logic, since different override sets can have implications down the line of the dependency chain, but it has the advantage of being consistent (instead of just going with the last edge in). Also, it raises an error if it encounters a real conflict, meaning two incoming edges with override sets that aren't just a subset of one another. So if we have dependency chains A->B->C and A->C, and we override C under B, then C will be overridden.

References

Fixes some of the override issues.

[wraithgar]

This is going to need tests

[wraithgar]

The edge's reload seems to me where this kind of thing should be happening. Is this a more subtle error perhaps where we're not reloading the overrides where we should?

[AlonNavon]

The edge's reload seems to me where this kind of thing should be happening. Is this a more subtle error perhaps where we're not reloading the overrides where we should?

There are several bugs in the mechanism, and this is just the first fix. Here I handle the case where a node has two incoming edges with different override sets, and I percolate it downwards in the dependency tree. So I'm not sure why it should be in the edge's reload. If you @wraithgar have time I can hop on a Google meet to discuss (and explain the other bugs).

[AlonNavon]

@wraithgar The linter and the tests are successful (except a random failure in macOS 18.0 which is unrelated to the PR).

[AlonNavon]

@wraithgar Anything else we need to do to merge this?

[sgarfinkel]

@AlonNavon This looks awesome, really looking forward to having overrides work more the way you'd expect. With this change, will running npm install re-apply any npm overrides as expected, or will running npm u still be required? Hopefully this gets approved soon 👍

[AlonNavon]

@sgarfinkel Hey, this is just the first fix. Concretely, there are at least 5 identifiable bugs. It fixes (1) and (2) and gives a reasonable solution for (3). I have other short PRs planned for (4) and (5), but first I want to merge this one. With those merged the behavior should be stable and correct (up to some freaky edge cases regarding the conflict resolution). But I haven't been able to get this one merged yet. Hopefully after the holidays it will get renewed attention. Every upvote counts.

The major bugs:

1. No percolation to the out-edges.
2. Not updating when deleting in-edges.
3. No conflict resolution of override sets (though ultimately, edges with conflicting overrides shouldn't be valid and be deduplicated IMHO).
4. A certain flow that updates to the parent's overrides which doesn't make sense.
5. Mishandling version ranges on edges.

[wraithgar]

Anything else we need to do to merge this?

Just patience. Holidays are over and we got a lot of PRs. This PR is on the work board it's not been forgotten.

[sgarfinkel]

Why was this closed?

[ljharb]

The PR wasn't made from the OP's fork, but presumably from their company's (which means that maintainers can't push to the PR branch, btw - always only make PRs from your own personal forks), and presumably someone at their company deleted the fork.

[AlonNavon]

Yeah, this was a mistake on our end. Our devops deleted the repo by accident. We restored the repo, but we need some maintainer with permissions to reopen the PR. @sgarfinkel @ljharb @wraithgar

[jbjhjm]

Can we have an update on this please? The PR seems to be on pause for months, but it adds important bugfixes. All lights are green, are there any problems remaining to be solved before merging it @wraithgar ?

[jakubmazanec]

@AlonNavon @ljharb What is the status on this PR? Can we please finally merge it? It's been 2,5 years since the original issue (#4232) , and we still have to perform complete clean npm install every time when we need to have some overrides (not mentioning the obvious security issues). Come on 🙏