fix(arborist): skip extraneous fsChildren in linked-strategy reify#9332
Open
manzoorwanijk wants to merge 1 commit intonpm:latestfrom
Open
fix(arborist): skip extraneous fsChildren in linked-strategy reify#9332manzoorwanijk wants to merge 1 commit intonpm:latestfrom
manzoorwanijk wants to merge 1 commit intonpm:latestfrom
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When using
install-strategy=linked, removing a workspace from the project (deleting its directory and dropping its entry from the rootpackage.json'sworkspacesarray) does not actually clean it up: the nextnpm installre-creates the deleted directory as<wsdir>/node_modules/<name> -> ..— an empty directory shell with a self-loop symlink and no source files.The hoisted (default) install strategy is unaffected: the directory stays gone.
The root cause is in
IsolatedReifier#makeIdealGraphinworkspaces/arborist/lib/arborist/isolated-reifier.js.On every install,
loadVirtualreads the previous lockfile and re-creates a node for every entry, including alink: trueentry for a workspace that has just been removed from the manifest.That node has no incoming workspace edge from the root, so
calcDepFlagscorrectly marks itextraneous, but it still ends up inidealTree.fsChildrenbecause its path is a descendant of the project root.makeIdealGraphthen iteratedfsChildrenblindly to populateidealGraph.workspaces, treating the orphan exactly like a real workspace.createIsolatedTreematerialised it as anIsolatedNodeplus anIsolatedLinkand, because the orphan is not in#rootDeclaredDeps, took the self-link branch — writing the workspace directory and thenode_modules/<name> -> ..symlink back to disk.Fixed by filtering out
extraneousfsChildrenbefore the sweep that populatesidealGraph.workspaces.A real workspace declared in
package.json'sworkspacesarray is reachable via aworkspace-type edge from the root, so it is neverextraneous.A local
file:dep target that legitimately lives inside the project tree is reachable via theLinkinnode_modules, so it is also neverextraneous.The only
fsChildrenentries that show up asextraneousare precisely the ghost workspaces left over in the lockfile after the user removed them — which is exactly what we want to skip.The sibling
queuewalk (which seedsidealGraph.externalfromedgesOut) is intentionally left alone: the orphan has no edges out and no incoming references, so it falls through theisLocalFileDep/externalbranches harmlessly.This keeps the change surgical to the one place where
fsChildrenis treated as the source of truth for "this is a workspace."Companion to #9330 (lockfile prune): with the prune fix in place, the orphan no longer appears in
package-lock.json'spackagesmap; with this fix in place, the linked strategy no longer re-materialises the orphan on disk.Either fix on its own is incomplete — the lockfile prune does not stop reify from re-creating the directory, and this fix does not stop the lockfile from carrying forward the entry.
The regression test seeds a project with two workspaces under
install-strategy=linked, removes one workspace's directory and its entry frompackage.json, runs reify again, and asserts the directory stays gone.The test fails before the fix (the directory and its self-loop symlink come back) and passes after.
References
Fixes #9331
Related to #9330, #5463