feat(BREAKING/install): require -- for script arg in deno install -g and support installing multiple packages#31292
Conversation
… <pkg2> ...` and require -- for script arg
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a breaking change to deno install -g by requiring the -- separator for script arguments and adding support for installing multiple packages in a single command, similar to npm install -g <pkg1> <pkg2>.
Key Changes:
- Changed
InstallFlagsGlobal.module_urlfrom a singleStringtomodule_urlsasVec<String>to support multiple packages - Added
script_arg()parsing with.last(true)to require--separator before script arguments - Added validation to prevent using
--nameflag when installing multiple packages
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/args/flags.rs | Updated flag definitions to use module_urls vector and added script argument parsing with -- separator requirement; includes test updates |
| cli/args/mod.rs | Modified to extract first URL from module_urls vector for CLI options |
| cli/tools/installer/mod.rs | Refactored installation logic to loop over multiple module URLs and updated function signatures; includes comprehensive test updates |
| tests/integration/install_tests.rs | Added test for multiple package installation and updated existing test to use -- separator |
| tests/specs/install/global/config_file_import_map/test.jsonc | Updated test to place script at end of command per new syntax requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: David Sherret <dsherret@users.noreply.github.com>
bartlomieju
left a comment
There was a problem hiding this comment.
LGTM, I agree this is desirable
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds support for installing multiple module URLs in a single global install invocation by changing InstallFlagsGlobal.module_url: String to module_urls: Vec. Parsing collects multiple module URLs and enforces that --name cannot be used when more than one package is specified. The global install flow now iterates per-module_url, updating shim resolution and error messages; create_install_shim and resolve_shim_data signatures were changed to accept an explicit module_url parameter. Tests and CLI wiring were updated to handle trailing script args via Sequence Diagram(s)sequenceDiagram
participant User as CLI
participant Parser as install_parse()
participant Validator as Validation
participant Installer as install_global()
participant Shim as create_install_shim()
User->>Parser: deno install -g module1 module2 -- script args
Parser->>Parser: Collect module_urls Vec<String>
Parser->>Validator: Validate flags (e.g. --name with multiple)
alt invalid: --name used with multiple
Validator-->>User: Error (InvalidValue)
else valid
Validator-->>Installer: InstallFlagsGlobal{module_urls: [...]}
loop for each module_url (index i)
Installer->>Installer: Ensure URL prefix or show hint (index-specific)
Installer->>Installer: load & type-check module graph for module_url
Installer->>Shim: create_install_shim(..., module_url)
Shim->>Shim: resolve_shim_data(..., module_url)
Shim-->>Installer: shim created
end
Installer-->>User: exit success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cli/args/flags.rs (3)
3153-3164: Consider constraining script args to--globalin install subcommandWiring
script_arg().last(true)intoinstall_subcommandenables the desireddeno install -g <pkg...> -- <script args...>flow, consistent with howbench/testhandle trailing script arguments. One nuance: this also allowsscript_argin non‑global installs, but those values are ignored in the local branch. If you want to avoid surprising “accepted but ignored” arguments fordeno installwithout-g, consider adding.requires("global")to thescript_argarg in this subcommand.
5865-5898: Global install parsing &--namerestriction are correct; consider one extra validationThe new global parsing that:
- collects
module_urlsfrom"cmd",- collects script args from
"script_arg", and- errors when
module_urls.len() > 1 && name.is_some()matches the PR goal (multi-package support plus
--before script args) and prevents ambiguous--nameusage. One behavioral point to consider: with the current shape,deno install -g a b -- foowill apply the sameargsslice (["foo"]) to all installed packages; if that’s not the intended UX, a follow-up could either reject combining multiple module URLs with script args or clearly document that args are shared across all installed binaries.
9788-9871: Install tests cover main paths; add a case for the new--nameerrorThe updated
installandinstall_with_flagstests verify:
- multi-package global installs populate
module_urlscorrectly, and- global installs with script args require
--and populateargsas expected.To lock in the new validation, it would be good to add a unit test that asserts
flags_from_vec(["deno", "install", "-g", "--name", "x", "a", "b"])produces the expectedInvalidValueerror message for--namewith multiple packages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cli/args/flags.rs(6 hunks)cli/args/mod.rs(1 hunks)cli/tools/installer/mod.rs(25 hunks)tests/integration/install_tests.rs(3 hunks)tests/specs/install/global/config_file_import_map/__test__.jsonc(1 hunks)
🔇 Additional comments (11)
cli/args/mod.rs (1)
1158-1162: LGTM! Clean adaptation to multi-module support.The change correctly accesses the first module URL from the new
module_urlsvector for implicit import permission handling. The use of.first()with.and_then()gracefully handles empty vectors by returningNone.tests/specs/install/global/config_file_import_map/__test__.jsonc (1)
3-3: LGTM! Test fixture updated to match new argument ordering.The module URL now appears after all flags, which aligns with the updated CLI parsing logic that supports multiple module URLs.
tests/integration/install_tests.rs (4)
257-258: LGTM! Correctly enforces the breaking change.The addition of
"--"before the script argument aligns with the PR objective of requiring explicit delimiter for script arguments indeno install -g.
297-297: LGTM! Consistent with the breaking change.The
--delimiter is correctly added before the script argumenthello, maintaining consistency with the updated CLI behavior.
433-455: LGTM! Excellent test coverage for multi-module installation.The test validates that:
- Multiple modules can be installed in a single command
- Both binaries are created successfully
- Success messages appear for each module
This comprehensively covers the new multi-module installation feature.
457-476: LGTM! Excellent migration guidance for users.This test validates the helpful error message for users migrating from Deno < 3.0, providing clear guidance on:
- The requirement for
--before script arguments- Alternative interpretation as a package with missing prefix
The error message strikes a good balance between addressing the breaking change and suggesting the correct usage.
cli/args/flags.rs (1)
299-305: InstallFlagsGlobal: module_urls vector design looks soundSwitching from a single
module_url: Stringtomodule_urls: Vec<String>cleanly enables multi-package global installs while keepingargs,root, andforcesemantics unchanged. No structural issues here.cli/tools/installer/mod.rs (4)
831-876: Validation logic is correct but uses indirect file existence check.The validation logic at line 831 checks
!cli_options.initial_cwd().join(entry_text).exists()to determine whether to provide helpful error messages. Whenentry_textis a URL (e.g., "http://..."), this check passes because the joined path won't exist locally, which triggers the prefix validation. While this works correctly, the indirection may not be immediately obvious to future maintainers.The migration error guidance (lines 833-846) and prefix suggestion logic (lines 848-875) provide excellent UX for common mistakes.
912-926: LGTM! Function signature appropriately parameterized.The signature change to accept
module_url: &strmakes the function's dependencies explicit and enables the multi-module installation flow. The function correctly creates a single install shim per invocation.
976-987: LGTM! Signature change aligns with multi-module support.The function signature correctly accepts
module_url: &strand uses it at line 987 for URL resolution. This parameterization is consistent with the refactoring approach and maintains clear data flow.
1210-1217: LGTM! Test helpers correctly updated for multi-module API.The test helper functions appropriately access
&install_flags_global.module_urls[0]since they're designed to test single-module installations. All test cases throughout the file (lines 1249, 1285, 1307, 1335, 1363, 1386-1388, 1415, 1446, 1482, 1514, 1547, 1584, 1619, 1648, 1668, 1691, 1721, 1750, 1790, 1835, 1877) have been consistently updated to usemodule_urls: vec![...], maintaining test coverage.Also applies to: 1230-1237
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/tools/installer/mod.rs (1)
838-908: Multi-URL loop, migration heuristics, and per-URL type-checking look sound; consider reusing the graph container.The new
for (i, module_url) in install_flags_global.module_urls.iter().enumerate()flow correctly:
- Distinguishes existing local paths, valid
jsr:/npm:/URL specs, and likely-misparsed script args (i == 1 && args.is_empty() && Url::parse(entry_text).is_err()), giving a clear 3.0 migration hint instead of a confusing failure.- Reuses the same
jsr_resolver/npm_resolverand runsload_and_type_check_filespluscreate_install_shimonce per URL, which matches the new “multi install” semantics.If you expect many packages in one invocation, you could optionally hoist
factory.main_module_graph_container().await?out of the loop and callload_and_type_check_fileson the reused container to avoid repeatedawait?overhead, but that’s a micro-optimization and not required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/tools/installer/mod.rs(25 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/tools/installer/mod.rs (2)
cli/factory.rs (3)
npm_resolver(645-648)bin_name_resolver(430-438)resolver(682-685)cli/tools/pm/mod.rs (1)
parse(787-860)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: bench release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug macos-x86_64
🔇 Additional comments (2)
cli/tools/installer/mod.rs (2)
912-926: Per-URL shim creation looks correct; double-check the invariant around--name+ multiple module URLs.Passing
module_url: &strintocreate_install_shim/resolve_shim_dataand then normalizing it withresolve_url_or_pathkeeps the previous behavior while making per-URL processing explicit. The lockfile logic (NpmPackageReqReference::from_specifier(&module_url)) and final shim args (executable_args.push(module_url.into())followed byinstall_flags_global.args) still behave as before, just on a per-entry basis, which is what you want for multi-install.One subtle invariant:
resolve_shim_datastill usesinstall_flags_global.nameas a single shared name. Ifmodule_urls.len() > 1andname.is_some(), you’d end up installing multiple URLs to the same shim path (either getting “Existing installation found” or silently overwriting with--force). The AI summary mentions this is enforced at the CLI arg layer; please confirm that:
- Either
InstallFlagsGlobalis constructed such thatname.is_some()impliesmodule_urls.len() == 1, or- You guard here and bail early with a dedicated error when both
name.is_some()andmodule_urls.len() > 1.If the CLI already guarantees the first, this is fine as-is; otherwise, adding a local assertion/early error here would make the behavior clearer.
Also applies to: 976-1045, 1117-1142
1200-1886: Tests and helpers correctly migrated tomodule_urlswhile preserving single-entry behavior.The test helpers now pass
&install_flags_global.module_urls[0]intocreate_install_shim/resolve_shim_data, and everyInstallFlagsGlobalin this module uses a single-entrymodule_urls: vec![…]. That keeps all existing tests exercising the same single-URL behavior they covered before, while aligning with the newVec<String>API. Nothing to change here.
Changes the behaviour of
deno install -gto require--for the script argument.This has the side effect of now supporting
deno install -g <pkg1> <pkg2>similar tonpm install -g.Closes #31024