This repository has been archived by the owner on Jan 20, 2022. It is now read-only.
add Arborist.rebuild() #83
Closed
Conversation
This file contains 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
isaacs
force-pushed
the
isaacs/rebuild
branch
3 times, most recently
from
e663f17
to
33ec372
Compare
There's no reason why these mixins have to stack on one another in the definition or extend one another. We only ever use the Arborist class as a fully-assembled thing, so it's fine to just throw them all in a list and call it a day.
For 'file:' deps (whether directory or tarball type), Node.resolved is always absolute. It was initially set up to be relative to the node path, because it had to be stashed back to package.json. But, then we got rid of that approach in favor of a more comprehensive lockfile, and so the added complexity of constantly re-evaluating the resolved value as the node moves around serves no purpose. This relative linking also _wasn't_ being handled properly in tarball dependencies, resulting in attempting to fetch a tarball from the wrong location, relative to the dependent's directory in node_modules, rather than where it should be found. Since we never store Node.resolved anywhere, there's no need for it to be portable, and the portability had a bug anyway. *Shrinkwrap* files *do* have to be portable, so the paths get relativized (and resolved) going into and out of lockfiles. But that's much simpler. Tarball dependencies are more consistently handled, and now that the Node.resolved value is absolute and predictable, it's easier to get that right. Tarball dependencies are properly regarded as a valid dependency resolution, provided that they come from the same target file. It's still a bit of a crapshoot when we get legacy install trees, but at least now we're not throwing out package._resolved when we load the manifest from pacote directly. The added test case installs a tarball dependency which depends on two other tarballs: 1 in its own containing folder, and another outside of its folder. (This exact situation was a thorn in npm's side for most of 6.x, and might still be in some cases.)
This refactors reify() to move bin linking and script running into the rebuild() method, and exposes it for use in 'npm rebuild'
ruyadorno
approved these changes
May 22, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Stacks on top of #81, land that first.
This refactors reify() to move bin linking and script running
into the rebuild() method, and exposes it for use in 'npm rebuild'