fix(arborist): omit failed optional dependencies from installed deps #8184
Conversation
|
@zkat I did what I could here. Had a couple of comments/lack of my own clarity in things I changed - see above. |
|
Other than the comment we really really should re-use the exceptional description from #8177, either by copying it into this issue now or when we merge. Probably easier to do now. I'll do it now. |
| @@ -1527,7 +1528,7 @@ This is a one-time fix-up, please be patient... | |||
|
|
|||
| const set = optionalSet(node) | |||
| for (const node of set) { | |||
| node.parent = null | |||
| node.ideallyInert = true | |||
There was a problem hiding this comment.
I've been thinking about this, and maybe it's better to just call it inert instead of ideallyInert. I think putting the ideally in there is probably a bit too much clever inside baseball and it's more verbose to boot. :)
Some other name would be good, too, as long as it communicates that this is in the tree, but we don't physically install it.
|
Thanks for pulling this work forward, much appreciated @zkat @owlstronaut @wraithgar. |
… 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).
f635c97 to
68a8d05
Compare
|
Amazing, I'm so excited for this to be fixed. Thank you @zkat @owlstronaut @wraithgar 🎉 |
|
@jakebailey Can this be back ported into old versions? |
|
This is not going to be backported. |
npm version 11.3.0 includes a fix (see: npm/cli#8184) that was pointed out in tailwindlabs/tailwindcss#15806 (comment) where it should resolve properly. See this thread for my own investigation parcel-bundler/lightningcss#956 (comment)
|
I still get this bug btw, despite updating node, npm, AND deleting my package-lock.json and node_modules and reinstalling. I even added this to package.json: The only way I got it to work is by doing this: which is dumb because i have to get all our devs to do it and all the other issues with deleting lockfiles and reinstalling everything. help! |
… 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
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