fix(arborist): omit failed optional dependencies from installed deps,…#8177
Closed
zkat wants to merge 3 commits intonpm:latestfrom
Closed
fix(arborist): omit failed optional dependencies from installed deps,…#8177zkat wants to merge 3 commits intonpm:latestfrom
zkat wants to merge 3 commits intonpm:latestfrom
Conversation
This was referenced Mar 22, 2025
Contributor
Author
|
Pretty sure all the test failures are just the tests needing to be updated because the nodes themselves don't go anywhere anymore. |
Contributor
Author
|
oh no it didn't quite work how embarrassing. Oh well see you monday :P |
Contributor
Author
|
probably need to fix load-actual-tree too or something. |
2 tasks
Contributor
Author
|
[cackle] It's aliiiiive! Now I just need to do see about round-tripping it. |
… but keep them 'in the tree' Fixes: npm#4828 Fixes: npm#7961 Replaces: npm#8127 Replaces: npm#8077 When optional dependencies fail, we don't want to install them, for whatever reason. The way this "uninstallation" is done right now is by simply removing the dependencies from the ideal tree during reification. Unfortunately, this means that what gets saved to the "hidden lockfile" is the edited ideal tree as well, and when NPM runs next, it'll use that when regenerating the "real" package-lock. This PR tags failed optional deps such that they're still added to the "trash list" (and thus have their directories cleaned up by the reifier), but prevents Arborist from removing them altogether from the ideal tree (which is usually done by setting their parent to `null`, making them unreachable, and letting them get GC'd).
Contributor
Author
|
Alright, this works now. No more thrashing, and the test suite is passing. There's 6 lines missing coverage, but as we talked about, I'll let you take care of it from here, @owlstronaut :) |
nick1udwig
added a commit
to hyperware-ai/hyperdrive
that referenced
this pull request
Mar 27, 2025
see fix implemented here, discussed at: npm/cli#4828 (comment) soon there will be a PR to address so we may be able to remove these optional dependencies once this PR is merged: npm/cli#8177
Contributor
Author
|
Work for this has moved to #8184, so I'm closing this! |
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
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.
… but keep them 'in the tree'
Fixes: #4828
Fixes: #7961
Replaces: #8127
Replaces: #8077
When optional dependencies fail, we don't want to install them, for whatever reason. The way this "uninstallation" is done right now is by simply removing the dependencies from the ideal tree during reification.
Unfortunately, this means that what gets saved to the "hidden lockfile" is the edited ideal tree as well, and when NPM runs next, it'll use that when regenerating the "real" package-lock.
This PR tags failed optional deps such that they're still added to the "trash list" (and thus have their directories cleaned up by the reifier), but prevents Arborist from removing them altogether from the ideal tree (which is usually done by setting their parent to
null, making them unreachable, and letting them get GC'd).PS: It's Friday, this is what I managed to get done together. I'm making this a draft PR for now so folks can look at it, but I want to make sure both reifiers work well, fix some messaging issues, and clean stuff up (I'm pretty sure I'm guarding
ideallyInertin more places than I need to, because I was trying to find the right spot). Still, feel free to talk about the approach. I'll get back to this on Monday.PPS: also hi