Don't prune optional dependencies #8077
Closed
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
|
@rotu I was looking into this PR. Am i correct that this is an attempt to fix #4828? The tests failing are regarding optional dependencies. With this PR any failing optional dependencies will remain in the the lock file, correct? Seems like the expected results need adjusting for these tests: Also, we should check out these pointers: #5282 (comment) |
This was referenced Feb 22, 2025
zkat
added a commit
to zkat/cli
that referenced
this pull request
Mar 22, 2025
… 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).
zkat
added a commit
to zkat/cli
that referenced
this pull request
Mar 22, 2025
… 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).
|
Hi. Just dropping a note that I'm trying to tackle this over at #8177 and I'm pretty confident in the approach but I don't be able to test in earnest until Monday. Some details aren't in place yet |
zkat
added a commit
to zkat/cli
that referenced
this pull request
Mar 24, 2025
… 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).
zkat
added a commit
to zkat/cli
that referenced
this pull request
Mar 26, 2025
… 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).
zkat
added a commit
to zkat/cli
that referenced
this pull request
Mar 26, 2025
… 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).
owlstronaut
pushed a commit
that referenced
this pull request
Mar 27, 2025
… 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).
owlstronaut
pushed a commit
that referenced
this pull request
Apr 1, 2025
… 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).
owlstronaut
added a commit
that referenced
this pull request
Apr 3, 2025
…8184) … but keep them 'in the tree' This PR was authored by @zkat 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 `ideallyInert` in 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 Co-authored-by: Kat Marchán <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
References