This repository has been archived by the owner on Jan 20, 2022. It is now read-only.
fix running unchanged links lifecycle scripts #290
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
ruyadorno
changed the title
isaacs
reviewed
Jun 17, 2021
ruyadorno
added a commit
to ruyadorno/arborist
that referenced
this pull request
Jun 25, 2021
Previously, the `node.isDescendantOf` method would only check for direct parents of that node, this may cause unexpected side effects on build ideal tree and reify when working with linked dependencies. This change modifies it so that `node.isDescendantOf` now looks at `node.resolveParent` (making sure `fsParents` are included in the lookup) to determine wether that node is a descendant of another. This also made it possible to refactor the logic that checks for an `external` dep in build-ideal-tree to use `node.isDescendantOf` instead of a regex check in the realpath of the link. Relates to: npm#290 Relates to: npm/cli#2900
Previously, the `node.isDescendantOf` method would only check for direct parents of that node, this may cause unexpected side effects on build ideal tree and reify when working with linked dependencies. This change modifies it so that `node.isDescendantOf` now looks at `node.resolveParent` (making sure `fsParents` are included in the lookup) to determine wether that node is a descendant of another. This also made it possible to refactor the logic that checks for an `external` dep in build-ideal-tree to use `node.isDescendantOf` instead of a regex check in the realpath of the link. Relates to: npm/cli#2900
ruyadorno
force-pushed
the
fix-run-unchanged-links-lifecycle-scripts
branch
2 times, most recently
from
ae6c2af to
2b35be1
Compare
Users expect workspaces (or regular linked deps) to run their prepare lifecycle script on every reify, since it's often used as some form of compilation step and should run even though that node hasn't changed. Fixes: npm/cli#2900
ruyadorno
force-pushed
the
fix-run-unchanged-links-lifecycle-scripts
branch
from
2b35be1 to
aedbc8c
Compare
isaacs
approved these changes
Jun 25, 2021
| // skip links that only live within node_modules as they are most | ||
| // likely managed by packages we installed, we only want to rebuild | ||
| // unchanged links we directly manage | ||
| if (node.isLink && node.target.fsTop === tree) |
There was a problem hiding this comment.
Could also consider || !this.diff.unchanged.includes(node.target.fsTop) to capture the "did not change link, but did change link's fsParent" use case. (Separate issue from the one this commit addresses, can come later.)
nlf
approved these changes
Jun 28, 2021
| @@ -914,7 +914,7 @@ This is a one-time fix-up, please be patient... | |||
| await Promise.all(promises) | |||
|
|
|||
| for (const { to } of node.edgesOut.values()) { | |||
| if (to && to.isLink) | |||
| if (to && to.isLink && to.target) | |||
There was a problem hiding this comment.
can something be a link and not have a target?
| @@ -1421,7 +1421,7 @@ This is a one-time fix-up, please be patient... | |||
| // prune anything deeper in the tree that can be replaced by this | |||
| if (this.idealTree) { | |||
| for (const node of this.idealTree.inventory.query('name', newDep.name)) { | |||
| if (node.isDescendantOf(target)) | |||
| if (!node.isTop && node.isDescendantOf(target)) | |||
There was a problem hiding this comment.
this extra check is because it's possible for isDescendantOf to return true when the node isTop? when is that the case? i'm wondering if it's worth putting a comment here explaining why both tests are needed, because i would sure assume that a node couldn't be both the descendant of a target and a top node
| @@ -1876,7 +1876,8 @@ This is a one-time fix-up, please be patient... | |||
| if (link.root !== this.idealTree) | |||
| continue | |||
|
|
|||
| const external = /^\.\.(\/|$)/.test(relpath(this.path, link.realpath)) | |||
| const tree = this.idealTree.target || this.idealTree | |||
| const external = !link.target.isDescendantOf(tree) | |||
There was a problem hiding this comment.
this is such a better test than the regex on the relative path, i love it
| const filter = node => | ||
| node.isDescendantOf(this.idealTree) && | ||
| node.top.isProjectRoot && |
There was a problem hiding this comment.
this is also much more clear to me than the old code
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.
Users expect workspaces (or regular linked deps) to run their prepare
lifecycle script on every reify, since it's often used as some form of
compilation step and should run even though that node hasn't changed.
References
Fixes: npm/cli#2900