From ca5e04c673f24d18aa46c4a3ebbcce0b3e333232 Mon Sep 17 00:00:00 2001 From: Parman Mohammadalizadeh Date: Sat, 18 Jul 2026 09:36:54 +0200 Subject: [PATCH] refactor(add): drop the vestigial Fn() -> DependencyGroupList generic from Add (#13107) refactor(add): drop the vestigial Fn() -> DependencyGroupList generic from Add list_dependency_groups existed only because the dependency-group list used to be consumed more than once. pnpm/pnpm#12995 collapsed that to a single collect-and-reuse, so the Fn() -> DependencyGroupList closure indirection no longer buys anything: take dependency_groups directly instead. Threads the same simplification through add_package/add_packages and their five call sites (add, global, dlx, runtime, self_update/install_pnpm), each of which drops a trivial closure/std::iter::once wrapper for a plain value or array literal. Pure internal refactor: no user-visible behavior change, so no changeset and no TypeScript counterpart. Closes pnpm/pnpm#13088 --- pnpm/crates/cli/src/cli_args/add.rs | 20 ++++++------- pnpm/crates/cli/src/cli_args/dlx.rs | 4 +-- pnpm/crates/cli/src/cli_args/global.rs | 4 +-- pnpm/crates/cli/src/cli_args/runtime.rs | 4 +-- pnpm/crates/cli/src/cli_args/runtime/tests.rs | 28 +++++++++++++++++++ .../src/cli_args/self_update/install_pnpm.rs | 4 +-- pnpm/crates/package-manager/src/add.rs | 13 ++++----- pnpm/crates/package-manager/src/add/tests.rs | 10 +++---- 8 files changed, 55 insertions(+), 32 deletions(-) diff --git a/pnpm/crates/cli/src/cli_args/add.rs b/pnpm/crates/cli/src/cli_args/add.rs index bb96b3c55f..2c3f55626a 100644 --- a/pnpm/crates/cli/src/cli_args/add.rs +++ b/pnpm/crates/cli/src/cli_args/add.rs @@ -203,14 +203,14 @@ impl AddArgs { let pinned_version = PinnedVersion::from_save_options(self.save_exact, self.save_prefix.as_deref()); - add_packages::( + add_packages::( state, &self.package_names, pinned_version, save_catalog_name, self.lockfile_only, supported_architectures, - || self.dependency_options.dependency_groups(), + self.dependency_options.dependency_groups(), ) .await } @@ -254,46 +254,44 @@ impl AddArgs { /// points `state` at a cache directory (via a [`Config`] whose `modules_dir` /// is anchored there) and saves to `dependencies` so the package's bin lands /// in `/node_modules/.bin`. -pub(crate) async fn add_package( +pub(crate) async fn add_package( state: State, package_name: &str, pinned_version: PinnedVersion, save_catalog_name: Option, lockfile_only: bool, supported_architectures: Option, - list_dependency_groups: ListDependencyGroups, + dependency_groups: DependencyGroupList, ) -> miette::Result<()> where Reporter: self::Reporter + 'static, - ListDependencyGroups: Fn() -> DependencyGroupList, DependencyGroupList: IntoIterator, { let package_names = [package_name.to_string()]; - Box::pin(add_packages::( + Box::pin(add_packages::( state, &package_names, pinned_version, save_catalog_name, lockfile_only, supported_architectures, - list_dependency_groups, + dependency_groups, )) .await } /// Add packages to `state`'s manifest and install them in one operation. -pub(crate) async fn add_packages( +pub(crate) async fn add_packages( mut state: State, package_names: &[String], pinned_version: PinnedVersion, save_catalog_name: Option, lockfile_only: bool, supported_architectures: Option, - list_dependency_groups: ListDependencyGroups, + dependency_groups: DependencyGroupList, ) -> miette::Result<()> where Reporter: self::Reporter + 'static, - ListDependencyGroups: Fn() -> DependencyGroupList, DependencyGroupList: IntoIterator, { // TODO: if a package already exists in another dependency group, don't remove the existing entry. @@ -312,7 +310,7 @@ where manifest, lockfile, lockfile_path: lockfile_path.as_deref(), - list_dependency_groups, + dependency_groups, package_names, pinned_version, save_catalog_name, diff --git a/pnpm/crates/cli/src/cli_args/dlx.rs b/pnpm/crates/cli/src/cli_args/dlx.rs index 7712c4f920..00f866ea5b 100644 --- a/pnpm/crates/cli/src/cli_args/dlx.rs +++ b/pnpm/crates/cli/src/cli_args/dlx.rs @@ -306,7 +306,7 @@ async fn install_into_cache( for pkg in pkgs { let state = State::init(manifest_path.clone(), config, false) .wrap_err("initialize the dlx install state")?; - add_package::( + add_package::( state, pkg, // dlx records the default caret range; the spec is throwaway. @@ -316,7 +316,7 @@ async fn install_into_cache( // dlx must download to run the bin, so never lockfile-only. false, config.supported_architectures.clone(), - || std::iter::once(DependencyGroup::Prod), + [DependencyGroup::Prod], ) .await?; } diff --git a/pnpm/crates/cli/src/cli_args/global.rs b/pnpm/crates/cli/src/cli_args/global.rs index 5051b0fb08..c90d996cec 100644 --- a/pnpm/crates/cli/src/cli_args/global.rs +++ b/pnpm/crates/cli/src/cli_args/global.rs @@ -359,14 +359,14 @@ async fn run_group_install( .collect::>>()?; let state = State::init(manifest_path, config, false) .wrap_err("initialize the global install state")?; - add_packages::( + add_packages::( state, &selectors, pinned_version, None, false, config.supported_architectures.clone(), - || std::iter::once(DependencyGroup::Prod), + [DependencyGroup::Prod], ) .await?; diff --git a/pnpm/crates/cli/src/cli_args/runtime.rs b/pnpm/crates/cli/src/cli_args/runtime.rs index 6a08f9c178..74f08fa25b 100644 --- a/pnpm/crates/cli/src/cli_args/runtime.rs +++ b/pnpm/crates/cli/src/cli_args/runtime.rs @@ -77,14 +77,14 @@ impl RuntimeArgs { /// `pnpm add @runtime:` in the project directory. pub async fn run(self, state: State) -> miette::Result<()> { let request = self.set_request()?; - add_package::( + add_package::( state, &request.package_name, PinnedVersion::Major, None, false, None, - || std::iter::once(request.dependency_group), + [request.dependency_group], ) .await } diff --git a/pnpm/crates/cli/src/cli_args/runtime/tests.rs b/pnpm/crates/cli/src/cli_args/runtime/tests.rs index bbde877018..38cf8b12af 100644 --- a/pnpm/crates/cli/src/cli_args/runtime/tests.rs +++ b/pnpm/crates/cli/src/cli_args/runtime/tests.rs @@ -1,6 +1,10 @@ +use pacquet_config::Config; use pacquet_package_manifest::DependencyGroup; +use pacquet_reporter::SilentReporter; +use tempfile::tempdir; use super::{RuntimeArgs, RuntimeError}; +use crate::State; fn args(params: &[&str]) -> RuntimeArgs { RuntimeArgs { @@ -93,6 +97,30 @@ fn set_request_accepts_every_supported_runtime() { } } +/// `run` (the local, non-`-g` path) must actually reach the shared +/// `add_package` pipeline with the request `set_request` built, not just +/// build the request and stop. `offline` makes the node resolver fail +/// fast (no network) as soon as it tries to resolve `node@runtime:22`, +/// which only happens once `run` has handed the request off. +#[tokio::test] +async fn run_hands_the_set_request_off_to_add_package() { + let dir = tempdir().expect("temp dir"); + let mut config = Config::new(); + config.store_dir = dir.path().join("pacquet-store").into(); + config.modules_dir = dir.path().join("node_modules"); + config.virtual_store_dir = config.modules_dir.join(".pacquet"); + config.offline = true; + let config = config.leak(); + let state = State::init(dir.path().join("package.json"), config, false).expect("init state"); + + let err = args(&["set", "node", "22"]) + .run::(state) + .await + .expect_err("offline resolution must fail, not silently succeed"); + let err = format!("{err:?}"); + assert!(err.contains("Offline"), "expected the offline Node.js resolver error, got: {err}"); +} + #[test] fn global_install_builds_the_same_runtime_selector() { // `--global` routes through `run_global`, which reuses `set_request` diff --git a/pnpm/crates/cli/src/cli_args/self_update/install_pnpm.rs b/pnpm/crates/cli/src/cli_args/self_update/install_pnpm.rs index 2cc6eaf90c..e27cfbedbd 100644 --- a/pnpm/crates/cli/src/cli_args/self_update/install_pnpm.rs +++ b/pnpm/crates/cli/src/cli_args/self_update/install_pnpm.rs @@ -288,14 +288,14 @@ pub(crate) async fn run_install( let manifest_path = install_dir.join("package.json"); let state = State::init(manifest_path, config, false) .wrap_err("initialize the self-update install state")?; - add_package::( + add_package::( state, &format!("{package_name}@{version}"), PinnedVersion::Patch, None, false, config.supported_architectures.clone(), - || std::iter::once(DependencyGroup::Prod), + [DependencyGroup::Prod], ) .await } diff --git a/pnpm/crates/package-manager/src/add.rs b/pnpm/crates/package-manager/src/add.rs index 9eb8a0cdad..2c02a5ed2f 100644 --- a/pnpm/crates/package-manager/src/add.rs +++ b/pnpm/crates/package-manager/src/add.rs @@ -30,9 +30,8 @@ use pacquet_tarball::MemCache; use pacquet_workspace_manifest_writer::{UpdateWorkspaceManifestError, update_workspace_manifest}; #[must_use] -pub struct Add<'a, ListDependencyGroups, DependencyGroupList> +pub struct Add<'a, DependencyGroupList> where - ListDependencyGroups: Fn() -> DependencyGroupList, DependencyGroupList: IntoIterator, { pub tarball_mem_cache: std::sync::Arc, @@ -43,7 +42,7 @@ where pub manifest: &'a mut PackageManifest, pub lockfile: Option<&'a Lockfile>, pub lockfile_path: Option<&'a std::path::Path>, - pub list_dependency_groups: ListDependencyGroups, + pub dependency_groups: DependencyGroupList, /// Package selectors, each of which may carry an `@` suffix. pub package_names: &'a [String], /// How the freshly-resolved version is pinned into the manifest range, @@ -127,9 +126,8 @@ pub enum AddError { MinimumReleaseAgeExclude(#[error(source)] pacquet_config::version_policy::VersionPolicyError), } -impl Add<'_, ListDependencyGroups, DependencyGroupList> +impl Add<'_, DependencyGroupList> where - ListDependencyGroups: Fn() -> DependencyGroupList, DependencyGroupList: IntoIterator, { pub async fn run(self) -> Result<(), AddError> { @@ -141,7 +139,7 @@ where manifest, lockfile, lockfile_path, - list_dependency_groups, + dependency_groups, package_names, pinned_version, save_catalog_name, @@ -167,8 +165,7 @@ where .map_err(AddError::InvalidCatalogsConfiguration)?; let prefix = workspace_dir_opt.as_deref().unwrap_or(&manifest_dir).to_string_lossy().into_owned(); - let dependency_groups: Vec = - list_dependency_groups().into_iter().collect(); + let dependency_groups: Vec = dependency_groups.into_iter().collect(); let latest_picker = tokio::sync::OnceCell::new(); let meta_cache = std::sync::Arc::new(InMemoryPackageMetaCache::default()); let fetch_locker = shared_packument_fetch_locker(); diff --git a/pnpm/crates/package-manager/src/add/tests.rs b/pnpm/crates/package-manager/src/add/tests.rs index 3e9f6b66a7..bacfac61b8 100644 --- a/pnpm/crates/package-manager/src/add/tests.rs +++ b/pnpm/crates/package-manager/src/add/tests.rs @@ -78,7 +78,7 @@ async fn add_routes_scoped_packages_to_configured_scoped_registry() { manifest: &mut manifest, lockfile: None, lockfile_path: None, - list_dependency_groups: || [DependencyGroup::Prod], + dependency_groups: [DependencyGroup::Prod], package_names: &package_names, pinned_version: PinnedVersion::Patch, save_catalog_name: None, @@ -201,7 +201,7 @@ async fn add_resolves_package_selectors_concurrently_and_reports_in_selector_ord manifest: &mut manifest, lockfile: None, lockfile_path: None, - list_dependency_groups: || [DependencyGroup::Prod], + dependency_groups: [DependencyGroup::Prod], package_names: &package_names, pinned_version: PinnedVersion::Patch, save_catalog_name: None, @@ -320,7 +320,7 @@ async fn add_reuses_shared_packument_state_for_every_selector_path() { manifest: &mut manifest, lockfile: None, lockfile_path: None, - list_dependency_groups: || [DependencyGroup::Prod], + dependency_groups: [DependencyGroup::Prod], package_names: &package_names, pinned_version: PinnedVersion::Patch, save_catalog_name: None, @@ -395,7 +395,7 @@ async fn add_reports_resolution_errors_in_selector_order() { manifest: &mut manifest, lockfile: None, lockfile_path: None, - list_dependency_groups: || [DependencyGroup::Prod], + dependency_groups: [DependencyGroup::Prod], package_names: &package_names, pinned_version: PinnedVersion::Patch, save_catalog_name: None, @@ -470,7 +470,7 @@ async fn add_does_not_wait_for_a_slower_later_resolution_after_an_error() { manifest: &mut manifest, lockfile: None, lockfile_path: None, - list_dependency_groups: || [DependencyGroup::Prod], + dependency_groups: [DependencyGroup::Prod], package_names: &package_names, pinned_version: PinnedVersion::Patch, save_catalog_name: None,