feat(package-manager): support version ranges and multiple packages in pacquet add#328
feat(package-manager): support version ranges and multiple packages in pacquet add#328dreygur wants to merge 5 commits into
pacquet add#328Conversation
|
Thanks for the contribution. If you want to work on some issues, you better choose something from the Stage 1 section in the roadmap: pnpm/pnpm#11633 We won't achieve our goals of shipping the rust fetching+linking into pnpm v12 if we will try to reimplement everything. The add command is only planned for Stage 2. |
There was a problem hiding this comment.
Pull request overview
Adds richer pacquet add argument handling so users can add multiple packages and specify exact versions, semver ranges, or dist-tags, while improving error propagation from registry fetches.
Changes:
- Accept multiple package arguments and parse
name[@specifier]including scoped forms. - Resolve specifiers into the version/range string saved to
package.json(including resolving dist-tags to concrete versions). - Extend registry URL tag handling with
PackageTag::Tag(String)and replace a panic with typed error propagation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/registry/src/package_tag.rs | Adds a Tag(String) variant for dist-tag URL segments. |
| crates/package-manager/src/add.rs | Implements multi-package add flow, specifier parsing/resolution, and adds unit tests for argument parsing. |
| crates/cli/src/cli_args/add.rs | Updates CLI to accept multiple package args and wires them into the package-manager Add flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Named distribution tag (for example `next` or `beta`). | ||
| #[display("{_0}")] | ||
| Tag(String), | ||
| } |
There was a problem hiding this comment.
PackageTag::Tag is added, but impl FromStr for PackageTag still only accepts latest or a semver Version and will return an error for named dist-tags like next/beta. If this type is meant to represent any registry URL tag, consider updating FromStr to parse unknown strings into PackageTag::Tag (and adjust the error type accordingly) to keep the API consistent with the new variant.
| #[display("Failed to fetch version for package: {_0}")] | ||
| FetchVersion(#[error(source)] RegistryError), |
There was a problem hiding this comment.
FetchVersion's display text says "for package" but it only formats the underlying RegistryError, so the message doesn't include which package/specifier failed and can be misleading. Consider storing the package name (and maybe the requested specifier/tag) on this error variant and include it in the display string (while keeping RegistryError as the source).
| let version = PackageVersion::fetch_from_registry( | ||
| name, | ||
| PackageTag::Tag(specifier.to_owned()), | ||
| http_client, | ||
| &config.registry, | ||
| ) |
There was a problem hiding this comment.
This code passes the parsed name directly into PackageVersion::fetch_from_registry, which currently constructs URLs as {registry}{name}/{tag} (see crates/registry/src/package_version.rs:33). For scoped packages (@scope/pkg), npm registries typically require URL-encoding the / as %2F (e.g. @scope%2Fpkg); without encoding, scoped installs can fail (404) depending on registry/proxy routing. Consider encoding the package name when building registry URLs (in the registry crate) so scoped packages work end-to-end.
|
Thanks. Will try to follow the roadmap for future contributions. |
Implements two passes that mirror pnpm's linkBinsOfPackages + linkAllBins from the headless restorer (3f37d17b23): 1. Root node_modules/.bin/ — symlinks for direct dependencies with has_bin. 2. Per-package node_modules/.bin/ inside each virtual-store slot — symlinks for that slot's own dependencies so lifecycle scripts can call them. Reads the bin field from each package's package.json (string and object forms), validates command names are URL-safe, guards against path traversal via lexical .. resolution, and ensures executable bits on targets. Upstream: https://github.com/pnpm/pnpm/blob/3f37d17b23/installing/deps-restorer/src/index.ts
Implements platform filtering for optional dependencies during frozen lockfile install, fixing issue pnpm#266. Packages with os/cpu/libc constraints in the lockfile packages: section are now skipped when the constraints do not match the current system and the package is only reachable via optional dependency edges. Two passes: 1. BFS from root importer tracks which snapshots are required vs. optional-only (required beats optional: a package reached via both paths is treated as required and always installed). 2. Optional-only snapshots that fail is_platform_supported() are added to skipped_snapshots, which CreateVirtualStore and SymlinkDirectDependencies both honour. Mirrors pnpm's checkPlatform in @pnpm/config.package-is-installable and the packageIsInstallable flow in the headless restorer: https://github.com/pnpm/pnpm/blob/3f37d17b23/config/package-is-installable/src/checkPlatform.ts supportedArchitectures (pnpm-workspace.yaml) is not yet implemented.
- Thread skipped_snapshots into LinkBins so platform-incompatible optional packages are skipped explicitly rather than relying on implicit NotFound. - Add directories.bin support via walkdir (mirrors getBinsFromPackageManifest). - Switch bin symlinks from absolute to relative targets, matching pnpm's symlink-dir behaviour. - Add bin conflict deduplication (deduplicateCommands): group by name, pick winner by ownership (BIN_OWNER_OVERRIDES), then alphabetical pkg name, then higher semver version. Mirrors @pnpm/bins.linker. - Add pkg_name/pkg_version to BinCmd for conflict resolution. Upstream references: - https://github.com/pnpm/pnpm/blob/3f37d17b23/bins/resolver/src/index.ts - https://github.com/pnpm/pnpm/blob/3f37d17b23/bins/linker/src/index.ts
Summary
name[@specifier]from each package argument, handling scoped packages(
@scope/pkg@1.0.0) correctly.package.json:latest→ fetch latest, save^x.y.z(or exact with--save-exact)1.2.3) → validate against registry, save^1.2.3^18,~1.0.0) → save as-is; install step resolves the best matchnext,beta) → fetch by tag, save^x.y.zPackageTag::Tag(String)variant so named dist tags produce the correctregistry URL (
{registry}/{name}/{tag}).pacquet add react react-dom@^18):pass runs.
.expect("resolve latest tag")with properAddError::FetchVersionpropagation.
Test plan
pacquet add <pkg>(no specifier) — existing behaviour unchangedpacquet add <pkg>@<version>— exact version saved as^x.y.zpacquet add <pkg>@^<range>— range saved as-ispacquet add <pkg>@<dist-tag>— resolved and saved as^x.y.zpacquet add <pkg1> <pkg2>— both added topackage.json, single install passparse_pkg_argcover plain, ranged, tagged, and scoped forms