feat(install): add --lockfile-only flag
#31376
Conversation
WalkthroughAdds a global --lockfile-only flag and threads it through add, install, remove/uninstall, outdated/update CLI flags and call sites. Refactors install flag types into InstallTopLevelFlags and InstallEntrypointsFlags and updates installer call signatures (install_from_entrypoints, install_top_level). Introduces CacheTopLevelDepsOptions to drive cache behavior and conditional resolution installs. Reworks the npm installer API to accept a single NpmInstallerOptions struct and adds install_resolution_if_pending / install_if_pending entry points. Adds tests for lockfile-only workflows and bumps workspace deno_npm from =0.42.0 to =0.42.1. Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Args as Args/Flags
participant Installer
participant NpmRes as NpmResolutionInstaller
participant Cache
User->>CLI: add --lockfile-only npm:pkg
CLI->>Args: parse --lockfile-only
Args-->>CLI: AddFlags{lockfile_only:true}
CLI->>Installer: install_from_entrypoints(flags, InstallEntrypointsFlags{entrypoints, lockfile_only:true})
Installer->>NpmRes: install_if_pending()
NpmRes->>Cache: resolve & write lockfile
Cache-->>NpmRes: done
NpmRes-->>Installer: resolution complete (no node_modules)
Installer-->>CLI: report success
CLI-->>User: deno.lock updated, node_modules untouched
sequenceDiagram
participant UpdateCmd
participant PM
participant CacheDeps as cache_top_level_deps
participant NPMInstall
UpdateCmd->>PM: OutdatedKind::Update{lockfile_only:true}
PM->>CacheDeps: cache_top_level_deps(CacheTopLevelDepsOptions{lockfile_only:true})
alt lockfile_only
CacheDeps->>NPMInstall: install_resolution_if_pending()
else
CacheDeps->>NPMInstall: cache_packages (full install)
end
NPMInstall-->>CacheDeps: done
CacheDeps-->>UpdateCmd: return
sequenceDiagram
participant Caller
participant Options as NpmInstallerOptions
participant NpmInstaller
Caller->>Options: construct NpmInstallerOptions{maybe_lockfile,...}
Caller->>NpmInstaller: NpmInstaller::new(install_reporter, Options)
NpmInstaller-->>Caller: initialized (supports install_resolution_if_pending)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Files/areas to focus on:
Pre-merge checks 10BC0 and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)cli/args/flags.rs (1)
🔇 Additional comments (2)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/args/flags.rs (1)
3352-3391: Wire--lockfile-onlyconsistently forupdate/uninstalland fix one outdated test expectationThere are a couple of inconsistencies between CLI wiring, parsers, and tests:
deno update --lockfile-only:
update_parsereadsmatches.get_flag("lockfile-only")viaoutdated_parse(OutdatedKind::Update haslockfile_only), and tests expect--lockfile-onlyto be supported forupdate(see theupdate_subcommandcases expectinglockfile_only: true).- However,
update_subcommand()only callsupdate_and_outdated_args()andlock_args(); it never registerslockfile-only, so Clap will treat--lockfile-onlyas an unknown argument and the test will fail.
deno uninstall --lockfile-only:
uninstall_parse()setsRemoveFlags { lockfile_only: matches.get_flag("lockfile-only") }for the local uninstall path, and the newuninstalltest with--lockfile-onlyexpects this to work.- But
uninstall_subcommand()never addslockfile_only_arg(), so--lockfile-onlyis again an unknown argument and can never reachuninstall_parse().Outdated test expecting
latest: truewithout--latest:
- In the
outdated_subcommandtests, the case for["--update", "--lockfile-only"]currently expectsOutdatedKind::Update { latest: true, interactive: false, lockfile_only: true }, butoutdated_parse()only setslatestbased on--latest. With no--latestflag,latestwill befalse, so this assertion is wrong.Concrete fixes:
- In
update_subcommand()(around Line 3352), register the flag:.defer(|cmd| { cmd .args(update_and_outdated_args()) + .arg(lockfile_only_arg()) .arg( Arg::new("interactive")
- In
uninstall_subcommand()(around Line 3446), register the flag for local uninstalls:.defer(|cmd| { cmd .arg(Arg::new("name-or-package").required_unless_present("help")) ... .arg( Arg::new("additional-packages") .help("List of additional packages to remove") .conflicts_with("global") .num_args(1..) .action(ArgAction::Append) ) .args(lock_args()) + .arg(lockfile_only_arg()) })
- In the
outdated_subcommandtests (around Line 13342), adjust the["--update", "--lockfile-only"]expectation to keeplatest: falseand only togglelockfile_only:- OutdatedFlags { - filters: vec![], - kind: OutdatedKind::Update { - latest: true, - interactive: false, - lockfile_only: true, - }, - recursive: false, - }, + OutdatedFlags { + filters: vec![], + kind: OutdatedKind::Update { + latest: false, + interactive: false, + lockfile_only: true, + }, + recursive: false, + },With these changes, the CLI, parse logic, and tests should all agree on
--lockfile-onlybehavior forupdateanduninstall, and the outdated test will reflect howlatestis actually derived.Also applies to: 3446-3487, 5400-5405, 5408-5436, 6117-6137, 10072-10107, 12872-12884, 13341-13352, 13488-13500
🧹 Nitpick comments (1)
cli/args/flags.rs (1)
3264-3269: Make--lockfile-onlyhelp text command-agnostic
lockfile_only_arg()’s help string is “Install only updating the lockfile”, but the same arg is reused ondeno add,deno remove,deno outdated --update, and (via shared parsing)deno update. For those commands it’s not “installing,” it’s “updating the lockfile without changing local installs.”Consider rewording to something neutral like “Only update the lockfile; do not change locally installed dependencies,” or give each subcommand its own brief help text if you want to be precise per verb.
Also applies to: 2055-2085, 2157-2182, 3166-3262, 3393-3443
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.locktests/specs/add/lockfile_only/install.outis excluded by!**/*.outtests/specs/add/lockfile_only/remove.outis excluded by!**/*.outtests/specs/install/lockfile_only/install.outis excluded by!**/*.outtests/specs/install/lockfile_only_entrypoint/install.outis excluded by!**/*.outtests/specs/outdated/deno_json/update_latest_lockfile_only/update.outis excluded by!**/*.out
📒 Files selected for processing (18)
Cargo.toml(1 hunks)cli/args/flags.rs(32 hunks)cli/args/mod.rs(1 hunks)cli/lsp/resolver.rs(2 hunks)cli/main.rs(1 hunks)cli/tools/installer/mod.rs(10 hunks)cli/tools/pm/cache_deps.rs(2 hunks)cli/tools/pm/mod.rs(5 hunks)cli/tools/pm/outdated/mod.rs(4 hunks)libs/npm_installer/factory.rs(3 hunks)libs/npm_installer/lib.rs(6 hunks)libs/npm_installer/resolution.rs(1 hunks)tests/specs/add/lockfile_only/__test__.jsonc(1 hunks)tests/specs/install/lockfile_only/__test__.jsonc(1 hunks)tests/specs/install/lockfile_only_entrypoint/__test__.jsonc(1 hunks)tests/specs/install/lockfile_only_entrypoint/deno.json(1 hunks)tests/specs/install/lockfile_only_entrypoint/main.ts(1 hunks)tests/specs/outdated/deno_json/__test__.jsonc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
cli/main.rs (1)
cli/tools/installer/mod.rs (1)
install_from_entrypoints(429-464)
cli/tools/pm/cache_deps.rs (3)
cli/lsp/resolver.rs (1)
npm_installer(1137-1139)libs/npm_installer/factory.rs (1)
npm_installer(345-385)cli/factory.rs (1)
npm_installer(641-643)
libs/npm_installer/resolution.rs (3)
libs/npm_installer/lib.rs (1)
add_package_reqs(265-275)libs/resolver/npm/managed/resolution.rs (4)
package_reqs(128-136)snapshot(107-112)snapshot(139-144)snapshot(171-173)cli/lsp/resolver.rs (2)
snapshot(194-266)snapshot(514-523)
cli/tools/pm/mod.rs (1)
cli/tools/pm/cache_deps.rs (1)
cache_top_level_deps(30-233)
cli/lsp/resolver.rs (3)
libs/npm_installer/factory.rs (1)
maybe_lockfile(233-241)cli/factory.rs (2)
maybe_lockfile(369-373)new(123-134)libs/resolver/factory.rs (1)
maybe_lockfile(479-518)
libs/npm_installer/lib.rs (3)
cli/factory.rs (4)
install_reporter(623-639)new(123-134)npm_cache(547-549)sys(517-519)libs/npm_installer/resolution.rs (1)
new(90-109)libs/npm_installer/factory.rs (3)
new(113-139)npm_cache(243-253)tarball_cache(402-418)
cli/tools/installer/mod.rs (3)
cli/factory.rs (2)
from_flags(340-347)emitter(729-731)cli/util/display.rs (3)
human_elapsed(51-53)human_elapsed_with_ms_limit(55-67)format(22-23)cli/tools/pm/cache_deps.rs (1)
cache_top_level_deps(30-233)
libs/npm_installer/factory.rs (2)
cli/factory.rs (1)
workspace_factory(650-673)libs/resolver/factory.rs (2)
workspace_factory(1152-1154)workspace_npm_link_packages(634-650)
🔇 Additional comments (19)
tests/specs/install/lockfile_only_entrypoint/deno.json (1)
1-3: LGTM!The configuration appropriately sets up manual node_modules handling for the lockfile-only entrypoint test scenario.
tests/specs/install/lockfile_only_entrypoint/main.ts (1)
1-1: LGTM!Simple test fixture that imports an npm package to exercise lockfile-only behavior with entrypoints.
tests/specs/install/lockfile_only_entrypoint/__test__.jsonc (1)
1-17: LGTM!Test specification comprehensively verifies that
--lockfile-onlywith entrypoints updates the lockfile without creating node_modules.tests/specs/install/lockfile_only/__test__.jsonc (1)
1-30: LGTM!Test specification thoroughly validates both lockfile-only mode (no node_modules) and normal install mode (creates node_modules).
tests/specs/add/lockfile_only/__test__.jsonc (1)
1-30: LGTM!Test specification comprehensively validates add and remove commands with
--lockfile-only, ensuring lockfile updates without node_modules creation.tests/specs/outdated/deno_json/__test__.jsonc (1)
115-134: LGTM!New test case validates
outdated --update --latest --lockfile-onlycommand. The output paths referencingupdate_latest(rather thanupdate_latest_lockfile_only) for deno.json and deno.lock comparisons suggest these files should be identical between modes.cli/args/mod.rs (1)
1374-1379: LGTM!Correctly extends the Manual caching strategy to include entrypoint installs with
lockfile_only: true, ensuring npm packages aren't installed locally when the flag is set.Cargo.toml (1)
74-74: No issues found.The deno_npm version "0.42.1" is valid and available on crates.io.
cli/tools/pm/cache_deps.rs (1)
26-28: Lockfile-only path correctly bypasses npm package cachingThe new
CacheTopLevelDepsOptions { lockfile_only }plus the conditional at the end cleanly preserves the old behavior whenlockfile_onlyis false and, when true, limits work toinstall_resolution_if_pending()instead ofcache_packages(PackageCaching::All), which is exactly what we want for a lockfile-only flow.Also applies to: 34-35, 222-228
cli/main.rs (1)
155-163: Cache subcommand wiring to InstallEntrypointsFlags looks correctPassing
InstallEntrypointsFlags { entrypoints: cache_flags.files, lockfile_only: false }is a straight adaptation to the new API and keepsdeno cachesemantics unchanged.cli/tools/pm/mod.rs (1)
44-46: lockfile_only is consistently threaded through add/remove into npm install flowRe-exporting
CacheTopLevelDepsOptions, extendingnpm_install_after_modificationto acceptcache_options, and populating it fromadd_flags.lockfile_only/remove_flags.lockfile_onlyensures the new CLI flag affects the cache/install behavior only where intended, while keeping the non-flagged path identical.Also applies to: 586-594, 916-924, 929-947
libs/npm_installer/factory.rs (1)
24-25: NpmInstallerOptions construction correctly replaces positional argumentsThe refactor to pass
NpmInstallerOptions { maybe_lockfile, maybe_node_modules_path, lifecycle_scripts, system_info, workspace_link_packages }intoNpmInstaller::newkeeps the same data flow as before, just structured, and should be behaviorally identical.Also applies to: 357-380
libs/npm_installer/resolution.rs (1)
119-123: install_if_pending + inner helper preserve behavior; confirm pending-flag contractThe split into
add_package_reqs_inner()looks good: it centralizes the queue lock, runsadd_package_reqs_to_snapshot(), and on success clears the pending flag and updates the snapshot.add_package_reqs()now simply delegates and convertsNpmResolutionErrorintoJsErrorBox, so its observable behavior is unchanged.install_if_pending()reuses the same path with an empty package list, which will only trigger real work when the resolution is in a state whereadd_package_reqs_to_snapshot()decides to run (for example, when marked pending).One thing worth double-checking is that every path which should cause
--lockfile-onlyoperations to recompute and write the lockfile is setting the resolution into that “pending” state beforeinstall_if_pending()is called; otherwise the empty-req call here will be a no-op. If your new tests coverdeno add/remove/... --lockfile-onlyend-to-end, that should validate the contract.Also applies to: 125-135, 137-155
cli/lsp/resolver.rs (1)
931-987: LSP NpmInstaller options wiring looks consistent and safeUsing
NpmInstallerOptionshere looks correct: LSP explicitly passesmaybe_lockfile: None, so it won’t mutate the project lockfile, while still wiringmaybe_node_modules_path,workspace_link_packages, and default lifecycle/system info through the structured options. This keeps previous behavior while matching the new constructor.cli/tools/pm/outdated/mod.rs (1)
18-18: Lockfile-only behavior fordeno outdated --updateis wired correctlyThe new
lockfile_onlyflag is threaded cleanly fromOutdatedKind::Updateintoupdateand then intonpm_install_after_modificationviaCacheTopLevelDepsOptions. That keeps the existing update logic while allowing lockfile-only resolution without touching installed npm packages.Also applies to: 252-267, 360-367, 436-440
cli/tools/installer/mod.rs (2)
207-215: Resolution reporting and “Resolved” summary align with lockfile-only behaviorUsing
package_req.to_string()inon_resolvedto populateresolved_npmand adding the “Resolved … in ” branch inprint_install_reportis a good fit for lockfile-only flows: when there are no npm/JSR downloads or initializations, users still see a clear summary of how many packages were resolved and how long it took, while install/download cases continue to use the existing “Installed/Downloaded” path.Also applies to: 584-595, 611-677
40-45: Structured flags and lockfile-only wiring for install flows look solidSwitching to
InstallEntrypointsFlags/InstallTopLevelFlagssimplifies the call sites, and havinginstall_top_levelown theCliFactoryconstruction and timing makes the API easier to use. Propagatingtop_level_flags.lockfile_onlyintoCacheTopLevelDepsOptionsensuresdeno install’s top-level mode honors the new lockfile-only semantics without changing existing behavior when the flag is false.Also applies to: 429-432, 437-439, 471-479, 740-780
libs/npm_installer/lib.rs (2)
148-154: NpmInstallerOptions refactor cleanly encapsulates installer configurationMoving
maybe_lockfile,maybe_node_modules_path, lifecycle script config, system info, and workspace link packages intoNpmInstallerOptionsand threading that throughNpmInstaller::newis a good cleanup: it shortens the constructor, keeps lockfile handling explicit on the installer viamaybe_lockfile, and cleanly separates local vs global installer setup based onmaybe_node_modules_pathwithout changing the observable behavior.Also applies to: 173-236
425-476:install_resolution_if_pendingis an appropriate hook for lockfile-only installsThe new
install_resolution_if_pendinghelper cleanly separates “finalize npm resolution state if there’s a pending snapshot” from package caching. Calling throughnpm_resolution_installer.install_if_pending()after ensuring initialization gives lockfile-only callers a way to update the resolution (and lockfile via the installer) without invokingcache_packages, which matches the intended semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/tools/installer/mod.rs (1)
591-593: Consider inlining or adding a comment.The local
human_elapsedhelper with a 3_000ms limit (vs. the global 1_000ms) is only used once. Consider either inlining the call todisplay::human_elapsed_with_ms_limit(elapsed.as_millis(), 3_000)directly at line 611, or adding a comment explaining why the different limit is needed for install reporting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tests/specs/add/lockfile_only/install.outis excluded by!**/*.outtests/specs/install/lockfile_only/install.outis excluded by!**/*.outtests/specs/install/lockfile_only_entrypoint/install.outis excluded by!**/*.out
📒 Files selected for processing (1)
cli/tools/installer/mod.rs(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/tools/installer/mod.rs (3)
cli/factory.rs (2)
from_flags(340-347)emitter(729-731)cli/util/display.rs (2)
human_elapsed(51-53)human_elapsed_with_ms_limit(55-67)cli/tools/pm/cache_deps.rs (1)
cache_top_level_deps(30-233)
🔇 Additional comments (5)
cli/tools/installer/mod.rs (5)
40-40: LGTM!New imports for the structured flag types are correctly added and used throughout the file.
Also applies to: 44-44
210-213: LGTM!The parameter rename and usage update correctly tracks the package requirement instead of leaving it unused.
429-464: LGTM!The refactor to accept structured
InstallEntrypointsFlagsis clean and the entrypoints are correctly accessed from the struct.
477-479: LGTM!The new match arm for TopLevel flags follows the existing pattern and correctly forwards parameters.
723-763: LGTM!The refactor correctly:
- Moves factory construction into the function for better encapsulation
- Threads the new
lockfile_onlyflag through tocache_top_level_deps- Improves timing variable naming and usage consistency
Adds a
--lockfile-onlyflag that only updates the lockfile and doesn't install any npm deps locally. Note that jsr and https dependencies are still cached because they require looking at the code to resolve dependencies.Applies to
deno install,deno add,deno resmove,deno outdated --update, anddeno updateCloses #31371