Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

fix: follow symlinks on build-ideal-tree add #129

Closed
wants to merge 2 commits into from

Conversation

ruyadorno
Copy link
Contributor

Symlinks were being tracked as invalid when loading actual trees since
their realpath wouldn't match, given that build-ideal-tree was just
preserving the fetchSpec read from npm-package-arg when adding new
packages to the ideal tree.

This change fixes it by properly parsing and following symlinks
realpaths at the moment they're added to the ideal tree.

Copy link
Contributor Author

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important thing to note is that I brought this code to my local cli and did a decent round of e2e testing, linking things to global space, linking from there into local projects and overall everything works as expected now 😁

@@ -55,7 +55,7 @@ const addSingle = ({pkg, spec, saveBundle, saveType, path}) => {
pkg[type] = pkg[type] || {}
if (rawSpec !== '' || pkg[type][name] === undefined) {
// if we're in global mode, file specs are based on cwd, not arb path
pkg[type][name] = specType === 'file' || specType === 'directory'
pkg[type][name] = specType === 'file'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was introduced in #125 as a way to compensate for the wrong parsed links and should not be necessary anymore

spec.name = mani.name
return spec
})
const currpath = this[_global] ? process.cwd() : this.path
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isaacs I decided to leave this here for now, we can revisit it later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can land on a separate commit, but ought to get it fixed, may as well pull in with this changeset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh, let's land it in a sep commit, maybe even better sep PR, let's thing about future us trying to bisect something 😂


spec.name = mani.name
return spec
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a small refactor for readability since when adding the _followSymlinkPath method (that is now below) things got gnarly in this part of the code

lib/arborist/build-ideal-tree.js Show resolved Hide resolved
Symlinks were being tracked as invalid when loading actual trees since
their realpath wouldn't match, given that build-ideal-tree was just
preserving the fetchSpec read from npm-package-arg when adding new
packages to the ideal tree.

This change fixes it by properly parsing and following symlinks
realpaths at the moment they're added to the ideal tree.
@ruyadorno ruyadorno force-pushed the fix-parse-symlink-follow-realpath branch from 0aaf65c to 5c50c0a Compare September 14, 2020 15:56
/* istanbul ignore else - same as the ignored catch above */
if (real) {
const { name } = spec
spec = npa(`file:${relpath(this.path, real)}`, this.path)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new spec gets npa parsed relative to this.path, so that's what will end up in resolved, in the package.json, etc - also something I want to validate that makes sense in the impl 😊

lib/add-rm-pkg-deps.js Outdated Show resolved Hide resolved
isaacs
isaacs previously requested changes Sep 14, 2020
Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit that we should use single shared rpcache and stcache members for buildIdealTree and loadActual.

Otherwise LGTM.

lib/arborist/build-ideal-tree.js Outdated Show resolved Hide resolved
lib/arborist/build-ideal-tree.js Outdated Show resolved Hide resolved
spec.name = mani.name
return spec
})
const currpath = this[_global] ? process.cwd() : this.path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can land on a separate commit, but ought to get it fixed, may as well pull in with this changeset.

lib/arborist/build-ideal-tree.js Outdated Show resolved Hide resolved
lib/arborist/build-ideal-tree.js Show resolved Hide resolved
@ruyadorno ruyadorno force-pushed the fix-parse-symlink-follow-realpath branch from 691f71b to 6ae40a7 Compare September 15, 2020 19:31
@ruyadorno ruyadorno dismissed isaacs’s stale review September 15, 2020 19:39

Approved over pair session review

@ruyadorno ruyadorno closed this in 1374d55 Sep 15, 2020
@darcyclarke darcyclarke added this to the OSS - Sprint 15 milestone Sep 25, 2020
@wraithgar wraithgar deleted the fix-parse-symlink-follow-realpath branch April 22, 2021 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants