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(arborist): inherit overrides from fsParent #8108

Closed

Conversation

TrevorBurnham
Copy link
Contributor

@TrevorBurnham TrevorBurnham commented Feb 14, 2025

Note to reviewers: The CI failures are unrelated to the code changes in this CR. They're caused by git dirty complaining because there was a release of @npmcli/git that hasn't been merged in yet: npm/git#224


Currently, a package's overrides have no effect on dependencies declared in its workspaces (#4205).

I did some digging and found that:

  1. In the virtual tree, workspace nodes are fsChildren of the root, not
    children.
  2. Nodes inherit overrides from their parent, but not from their fsParent.

As a result, workspace nodes in the virtual tree weren't inheriting the root's overrides.

The fix here is to allow nodes to inherit fsParent.overrides. I've added tests for load-virtual and reify to demonstrate that overrides are correctly applied to a workspace dependency with this change.

References

Fixes #4205

@TrevorBurnham TrevorBurnham requested a review from a team as a code owner February 14, 2025 03:46
@TrevorBurnham TrevorBurnham changed the title Inherit overrides from fsParent fix: Inherit overrides from fsParent Feb 14, 2025
@TrevorBurnham TrevorBurnham changed the title fix: Inherit overrides from fsParent fix: inherit overrides from fsParent Feb 14, 2025
@TrevorBurnham TrevorBurnham changed the title fix: inherit overrides from fsParent fix(arborist): inherit overrides from fsParent Feb 14, 2025
@TrevorBurnham TrevorBurnham force-pushed the fix-workspace-overrides branch from 1f64523 to e83ca91 Compare February 14, 2025 03:51
@TrevorBurnham TrevorBurnham force-pushed the fix-workspace-overrides branch 2 times, most recently from 6d5d51e to e70cfb8 Compare February 14, 2025 15:48
@wraithgar
Copy link
Member

CI failures are hopefully fixed in #8115

This PR has been added to npm/statusboard#920, and @owlstronaut will make sure it's not colliding w/ existing work there.



The tests show that the virtual tree ignores overrides on workspace
dependencies, causing the corresponding nodes to have only their spec
versions after reification.
Fixes npm#4205

In the virtual tree, workspace nodes are `fsChildren` of the root, not
`children`. As a result, they weren't inheriting the root's overrides,
causing those overrides to be ignored for all dependencies of
workspaces. The fix here is to inherit `fsParent.overrides` whenever
possible.
@TrevorBurnham TrevorBurnham force-pushed the fix-workspace-overrides branch from e70cfb8 to 7e510a1 Compare February 18, 2025 19:15
@owlstronaut
Copy link
Contributor

owlstronaut commented Feb 21, 2025

Hey @TrevorBurnham! Thanks for opening this. Just a heads-up that we’re trying to land #8089, which reworks how overrides propagate from parents to children—shifting the logic to the edges rather than at the node level. Because that PR removes the exact code you’re modifying here, your changes may no longer be applicable once it lands. As such, I'll close this. Still appreciate your contribution, and feel free to weigh in if you have any questions or concerns!

@TrevorBurnham
Copy link
Contributor Author

TrevorBurnham commented Feb 21, 2025

@owlstronaut Nice, I've confirmed that the test cases in e8bd8bc pass against #8089. 👍 Feel free to cherry-pick them if you agree that they demonstrate the correct behavior; I think it'd be helpful to have direct test coverage for workspace overrides.

@owlstronaut
Copy link
Contributor

@owlstronaut Nice, I've confirmed that the test cases in e8bd8bc pass against #8089. 👍 Feel free to cherry-pick them if you agree that they demonstrate the correct behavior; I think it'd be helpful to have direct test coverage for workspace overrides.

Wonderful, thank you! I've got that on my radar

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.

[BUG] overrides not working as expected in workspace
3 participants