fix(arborist): improve override conflict detection with semantic comparison #8689
+444
−72
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The override conflict detection added in b9225e5 is overly conservative and produces false positives in two scenarios: 1. Peer dependencies: These resolve to nodes in different parts of the tree with legitimately different override contexts. 2. Reference overrides ($syntax): Different override sets using references (e.g., $@vaadin/react-components vs $@vaadin/react-components-pro) are structurally different but functionally equivalent when they resolve to the same versions. This fix removes the override conflict check entirely from edge validation. The check was redundant because: - Version satisfaction is already validated by satisfiedBy() - Any actual version conflicts in dependencies are caught during normal dependency resolution in the build/reify phase - The check caused false positives that prevented valid dependency configurations from working Fixes: npm#8688
…ation because it caused false positives with reference overrides ($syntax) that resolve to functionally equivalent versions. Real conflicts are caught during the build/reify phase. See issue npm#8688 and the fix in edge.js
Replace the test 'should find inconsistency between the edge's override set and the target's override set' which was testing the override conflict detection code that was intentionally removed. The new test 'edges with different override contexts to same node should be valid' is a regression test for issue npm#8688. It verifies that edges remain valid when the edge and target node have different override contexts, as long as the version requirements are satisfied. The override conflict check (from b9225e5) was causing false positives, especially with reference overrides ($syntax) that resolve to functionally equivalent versions despite being structurally different. The fix removes this check because: 1. satisfiedBy() already validates version requirements 2. Real conflicts are caught during build/reify phase 3. The check compared override sets structurally, not functionally
|
This really needs to be resolved before Node 24 becomes LTS. Basic functionality like |
|
I'm not sure that removing the check altogether is what we need here. Shouldn't the fix be in the set detection itself? This seems like just ignoring a thing that's giving us an error to make the error go away, rather than fixing the part that's making it error. |
|
I am not familiar enough with the code to comment but if you can give some pointers, I can try to make the fix better |
…arison Instead of removing override conflict detection entirely, enhance it to use semantic comparison of version requirements rather than pure structural equality checking.
|
So |
Artur-
changed the title
|
Maybe @owlstronaut who did the original PR has some comments? |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves the issue where different override contexts (like Vaadin's$@vaadin/react-components vs $ @vaadin/react-components-pro) were incorrectly treated as conflicts by structural comparison.
Fixes #8688