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: issue-5850 - Settle override conflicts between edges and propagate changes #7025

Open
wants to merge 33 commits into
base: latest
Choose a base branch
from

Conversation

AlonNavon
Copy link

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.

@AlonNavon AlonNavon requested a review from a team as a code owner November 26, 2023 13:37
@AlonNavon AlonNavon changed the title Settle overrides conflicts between edges, and propagate changes to the edges out fix: issue-5850 - Settle overrides conflicts between edges, and propagate changes to the edges out Nov 26, 2023
@wraithgar wraithgar self-assigned this Nov 28, 2023
@wraithgar
Copy link
Member

This is going to need tests

@wraithgar
Copy link
Member

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
Copy link
Author

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 AlonNavon changed the title fix: issue-5850 - Settle overrides conflicts between edges, and propagate changes to the edges out fix: issue-5850 - Settle overrides conflicts between edges, and propagate changes Dec 7, 2023
@AlonNavon AlonNavon changed the title fix: issue-5850 - Settle overrides conflicts between edges, and propagate changes fix: issue-5850 - Settle override conflicts between edges and propagate changes Dec 7, 2023
@AlonNavon
Copy link
Author

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

@AlonNavon
Copy link
Author

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

@sgarfinkel
Copy link

@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
Copy link
Author

AlonNavon commented Dec 26, 2023

@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
Copy link
Member

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
Copy link

Why was this closed?

@ljharb
Copy link
Contributor

ljharb commented Mar 14, 2024

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
Copy link
Author

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

jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 6, 2024
jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 6, 2024
jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 7, 2024
jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 7, 2024
jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 7, 2024
@@ -142,6 +181,24 @@ class OverrideSet {

return ruleset
}

static findSpecificOverrideSet (first, second) {
Copy link
Author

Choose a reason for hiding this comment

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

@jdalton I moved the functions here, because fundamentally they are about comparing override sets, so this is the most natural location for them I think.

Copy link
Author

Choose a reason for hiding this comment

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

Is there anything else, or do you approve the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@AlonNavon
I think it looks great. Could you add code comments inline explaining the cases when the .silly is called (the one you changed from throwing to .silly). This will help folks if they see the logging know why.

Then the only other thing is adding any unit tests to cover some of the things you discovered and tweaked.

Copy link
Author

Choose a reason for hiding this comment

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

Hey @wraithgar, can you run the tests? To make sure nothing got accidentally broken with all the changes.

Copy link
Author

Choose a reason for hiding this comment

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

@jdalton For the override set functions I created unit tests. I'm not sure how to translate the test cases that I'm running to unit tests.
This is the package.json of the simplest case:
{ "name": "test", "version": "1.0.0", "engines": { "npm": ">=8.3.0" }, "dependencies": { "json-server": "0.17.0" }, "overrides": { "json-server": { "package-json": "7.0.0" } } }

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me. @wraithgar I think this is where we'll need your help to push it over the line. (one last test case and a review). 🎉

Copy link
Contributor

@jdalton jdalton Dec 11, 2024

Choose a reason for hiding this comment

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

@AlonNavon

Heads up! Socket folks just noticed an issue with the lock files being generated and npm ci.

In Socket's case the new patched version of npm DOES produce a technically correct lock file, however the npm ci command is expecting an incorrect lock file and errors when the correct lock file does NOT conform.

The issue is around dealing with bundled dependencies. For example, the package @socketregistry/deep-equal has bundled dependencies which with this patch get correctly resolved and represented in the lock file. However, npm ci expects the lock file to be layedout in a way which ignores the bundled dep and looks for the dep hoisted in the higher node_modules folder even though it DOES NOT exist on disk.

All of this to say, I think there is something in npm ci that needs checking in on (I believe it does do a form of npm install). The workaround for Socket was to do the "fixed" install and then run npm install --ignore-scripts --package-lock-only to put back the incorrect, but expected by npm ci, lock entries. The --ignore-scripts --package-lock-only worked on the patched npm too. So something to investigate.

@wraithgar
Copy link
Member

👋 We are shipping npm 11 this morning and will be able to focus on this issue again. This is a priority task for us this next quarter.

@wraithgar wraithgar closed this Dec 16, 2024
@wraithgar wraithgar reopened this Dec 16, 2024
@wraithgar
Copy link
Member

Had to close/reopen to get the "approve" button to show up again.

@AlonNavon
Copy link
Author

👋 We are shipping npm 11 this morning and will be able to focus on this issue again. This is a priority task for us this next quarter.

That's great news! I really hope we can get this fix deployed soon 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants