Eliminate mv, ncp, wrench dependencies #164
Open
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
There is no API breakage here because neither ncp nor the mv default imported functions ever returned called the callback with anything but an error. Important lesson from the spectests: the copy we are aiming for is with "verbatim symlinks", ie. when encountering a symlink during copying, do **not** reify it, just create a symlink with the same target. Apparently this is how it worked. Without setting this right, the install command would fail some tests in funky ways.
Since we generally aim towards the async APIs that didn't even exist at the original writing of apm, I chose to replace this synchronous API with a Promise-based one and made the appropriate changes at the call site; relatively small modification in init.js
This certainly could be done nicer but this is the simplest incremental modification - mv is now gone for good, so long, Mr. Andrew Kelley, and thanks for all the fish!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Resolves #157. Comments might be worth reading.
This is meant to be a simple and incremental refactor with relatively meaningful commits. All spectests pass after the modifications, and 3 direct dependencies have been eliminated in favor of Node's built-in
fs/promisesAPI.As I mentioned before,
fsPromises.cpis an experimental utility in Node 20 - however, I see no reason to come up with an intermediary solution when Node 20 itself is basically "oldstable" now and Node 22 promoted the same API to stable. I wouldn't expect any sort of trouble but in any case, this clearly seems to be a straight path.