fix(npm): clean node_modules after deno remove#34110
Merged
Merged
Conversation
fibibot
changed the title
Contributor
Author
|
CI is green. @bartlomieju this is ready for review. |
bartlomieju
reviewed
Jun 1, 2026
bartlomieju
left a comment
bartlomieju
left a comment
Member
There was a problem hiding this comment.
Reviewed this — solid fix, root cause is correctly diagnosed and the three changes hang together well.
Correctness (traced by hand, looks right):
node_modules_package_actual_dir_to_name: the real bug. For a scoped target like.deno/@[email protected]/node_modules/@denotest/addthe old two-.parent()walk returned"node_modules"instead of the folder-id, so it never matchedkeep_names. The new grandparent-is-node_modulescheck is correct, and the added unit test covers both scoped and unscoped.remove_unused_node_modules_symlinks: scoped symlinks live under a@scope/subdir that the old top-level-only loop skipped (the dir itself isn't a symlink). Recursing into@-prefixed dirs and pruning the empty scope dir is the right fix;.deno/.binare.-prefixed so they're untouched.- The early-return changes in
local.rs/hoisted.rsplus thelib.rscache_packagestrigger correctly handle removing the last package (empty snapshot used to bail before cleanup, andmaybe_cache_packagesshort-circuited onuncached.is_empty()).
Points before merge (non-blocking):
hoisted.rschanges aren't covered by a test. The spec test usesnodeModulesDir: "auto"(isolated layout →local.rs), so the scoped cleanup added tocleanup_hoisted_packagesis never exercised. Could you add a hoisted-layout case, or confirm it was manually verified? In particular theexpected_names@scope/pkgformat assumption.lib.rsperf nuance: everyPackageCaching::Allinstall where everything is already cached now re-runscache_packages→sync_resolution_with_fsinstead of short-circuiting. It's guarded bysetup_cachedownstream so should be cheap, but it's a behavior change on the install hot path worth a line in the description.- nit: after removing the last package,
fs_create_dir_allrecreates an emptynode_modules/.deno/node_modules, leaving an emptynode_modules/.denobehind. The test only asserts specific entries are gone, not thatnode_modulesis fully cleaned.
Overall LGTM pending the hoisted test coverage.
Add a sibling spec test that exercises deno remove cleanup under the hoisted node_modules linker, covering the scoped-package removal and empty scope-directory pruning in cleanup_hoisted_packages. Document why maybe_cache_packages re-syncs node_modules for All caching even when every package is already cached.
littledivy
pushed a commit
to crowlKats/deno
that referenced
this pull request
Jun 10, 2026
Fixes denoland#34073. This updates the local npm installer cleanup path so `deno remove` can reconcile an existing managed `node_modules` even when the updated npm resolution has zero packages. It also cleans scoped package entries in both isolated and hoisted layouts. The regression spec now uses npm imports in `deno.json` with `nodeModulesDir: "auto"`, removes a scoped package, then removes the last package and checks `node_modules`, `deno.json`, and `deno.lock`. --------- Co-authored-by: fibibot <[email protected]> Co-authored-by: Bartek Iwańczuk <[email protected]>
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
2 participants
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.
Fixes #34073.
Closes bartlomieju/orchid-inbox#101
This updates the local npm installer cleanup path so
deno removecan reconcile an existing managednode_moduleseven when the updated npm resolution has zero packages. It also cleans scoped package entries in both isolated and hoisted layouts.The regression spec now uses npm imports in
deno.jsonwithnodeModulesDir: "auto", removes a scoped package, then removes the last package and checksnode_modules,deno.json, anddeno.lock.Verification:
deno fmt --check tests/specs/remove/node_modules_cleanup/__test__.jsonc tests/specs/remove/node_modules_cleanup/deno.jsongit diff --check./x test-spec remove/node_modules_cleanupbecause this VM does not havecargoinstalled../x fmt --checkbecause this VM does not haverustfmtinstalled.AI disclosure: This PR was authored with assistance from OpenAI Codex.