diff --git a/.changeset/peer-closures-root-context.md b/.changeset/peer-closures-root-context.md new file mode 100644 index 0000000000..d682c8f092 --- /dev/null +++ b/.changeset/peer-closures-root-context.md @@ -0,0 +1,6 @@ +--- +"@pnpm/installing.deps-resolver": patch +"pnpm": patch +--- + +Fixed peer dependency resolution with `autoInstallPeers` when a workspace package depends on a version of a package that a transitive dependency's self-contained closure also provides for itself. The peer providers that are attached to the root project for reuse are no longer peer-resolved a second time in the root context, so packages inside such a closure no longer get their peers bound to the root project's incompatible version [#4993](https://github.com/pnpm/pnpm/issues/4993). diff --git a/pacquet/crates/package-manager/src/install/tests.rs b/pacquet/crates/package-manager/src/install/tests.rs index 7235ad8684..5a48e3d4ad 100644 --- a/pacquet/crates/package-manager/src/install/tests.rs +++ b/pacquet/crates/package-manager/src/install/tests.rs @@ -1720,6 +1720,103 @@ async fn auto_install_peers_skips_meta_only_optional_peers() { drop((dir, mock_instance)); } +/// Mirror of the TS test "a root dependency does not override the +/// peers provided inside a self-contained subtree" +/// (`deps-installer/test/install/autoInstallPeers.ts`). +/// +/// `@pnpm.e2e/closure-plugins` provides every peer of its own subtree +/// (`closure-lib-a` and `closure-lib-b` peer-depend on each other and +/// on `closure-peer-x`, and all three are its regular dependencies). +/// The root project additionally depends on the incompatible +/// `closure-peer-x@2.0.0`. With `autoInstallPeers` (the default), the +/// peers resolved inside the subtree are attached to the root project +/// for reuse — but they must not be peer-resolved again in the root +/// context, where `closure-peer-x@2.0.0` is the nearest provider, or +/// the subtree's peer graph gets a mix of both versions. +#[tokio::test] +async fn root_dependency_does_not_override_peers_of_self_contained_subtree() { + let mock_instance = TestRegistry::start(); + + let dir = tempdir().unwrap(); + let store_dir = dir.path().join("pacquet-store"); + let project_root = dir.path().join("project"); + let modules_dir = project_root.join("node_modules"); + let virtual_store_dir = modules_dir.join(".pacquet"); + + let manifest_path = dir.path().join("package.json"); + let mut manifest = PackageManifest::create_if_needed(manifest_path.clone()).unwrap(); + manifest.add_dependency("@pnpm.e2e/closure-plugins", "1.0.0", DependencyGroup::Prod).unwrap(); + manifest.add_dependency("@pnpm.e2e/closure-peer-x", "2.0.0", DependencyGroup::Prod).unwrap(); + manifest.save().unwrap(); + + let mut config = Config::new(); + config.store_dir = store_dir.into(); + config.modules_dir = modules_dir.clone(); + config.virtual_store_dir = virtual_store_dir; + config.registry = mock_instance.url(); + let config = config.leak(); + + Install { + tarball_mem_cache: Default::default(), + http_client: &Default::default(), + http_client_arc: std::sync::Arc::new(Default::default()), + config, + manifest: &manifest, + emit_initial_manifest: true, + lockfile: MaybeLazyLockfile::Loaded(None), + lockfile_path: None, + dependency_groups: [DependencyGroup::Prod, DependencyGroup::Dev, DependencyGroup::Optional], + frozen_lockfile: false, + prefer_frozen_lockfile: None, + ignore_manifest_check: false, + skip_runtimes: false, + trust_lockfile: false, + update_checksums: false, + is_full_install: true, + supported_architectures: None, + node_linker: pacquet_config::NodeLinker::default(), + lockfile_only: false, + dry_run: false, + resolved_packages: &Default::default(), + update_seed_policy: crate::UpdateSeedPolicy::KeepAll, + auth_override: None, + resolution_observer: None, + catalogs_override: None, + disable_optimistic_repeat_install: false, + pnpmfile_hook_override: None, + workspace_projects_override: None, + } + .run::() + .await + .expect("install should succeed"); + + let content = + std::fs::read_to_string(dir.path().join(Lockfile::FILE_NAME)).expect("read pnpm-lock.yaml"); + assert!( + !content.contains("(@pnpm.e2e/closure-peer-x@2.0.0)"), + "no peer inside the self-contained subtree may bind to the root's \ + closure-peer-x@2.0.0; lockfile:\n{content}", + ); + assert!( + content.contains("(@pnpm.e2e/closure-peer-x@1.0.0)"), + "the subtree's peers must bind to its own closure-peer-x@1.0.0; lockfile:\n{content}", + ); + + // The root keeps its own explicitly declared version. + let lockfile: Lockfile = serde_saphyr::from_str(&content).expect("parse pnpm-lock.yaml"); + let root_deps = lockfile + .root_project() + .expect("root importer recorded") + .dependencies + .as_ref() + .expect("dependencies map"); + let peer_x_key = pacquet_lockfile::PkgName::parse("@pnpm.e2e/closure-peer-x").unwrap(); + let root_peer_x = root_deps.get(&peer_x_key).expect("closure-peer-x recorded at root"); + assert_eq!(root_peer_x.version.to_string(), "2.0.0"); + + drop((dir, mock_instance)); +} + /// A v9 lockfile fixture pinned to a placeholder package whose /// integrity is bogus on purpose. Pacquet enforces tarball integrity /// on the install path, so any test that lets the install reach the diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs index 697a9d2e87..67c9bbca46 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs @@ -285,6 +285,12 @@ pub(crate) struct ImporterHoistState { importer_id: String, ctx: TreeCtx, direct: Vec, + /// `NodeIds` appended to `direct` by + /// [`Self::append_resolved_peer_providers`]. Threaded into + /// [`ResolvePeersOptions::hoisted_peer_provider_node_ids`] so the + /// peer walk resolves them at their tree position instead of the + /// importer root. + hoisted_peer_provider_node_ids: HashSet, parent_pkg_aliases: HashSet, all_missing_optional_peers: BTreeMap>, all_preferred_versions: PreferredVersions, @@ -357,6 +363,7 @@ impl ImporterHoistState { importer_id: importer_id.to_string(), ctx, direct, + hoisted_peer_provider_node_ids: HashSet::new(), parent_pkg_aliases, all_missing_optional_peers: BTreeMap::new(), all_preferred_versions, @@ -379,6 +386,7 @@ impl ImporterHoistState { lockfile_dir: self.lockfile_dir.clone(), modules_dir: self.modules_dir.clone(), hoist_missing_scope: None, + hoisted_peer_provider_node_ids: self.hoisted_peer_provider_node_ids.clone(), } } @@ -517,6 +525,7 @@ impl ImporterHoistState { node_id: node_id.clone(), id: tree_node.resolved_package_id.clone(), }); + self.hoisted_peer_provider_node_ids.insert(node_id.clone()); self.parent_pkg_aliases.insert(alias.clone()); } } @@ -558,9 +567,11 @@ impl ImporterHoistState { /// The importer's direct-dep envelopes for the workspace-wide peer /// pass (which recomputes peers across importers; the per-importer - /// pass would be discarded). - pub(crate) fn into_direct(self) -> Vec { - self.direct + /// pass would be discarded), together with the `NodeIds` of the peer + /// providers among them (see + /// [`ResolvePeersOptions::hoisted_peer_provider_node_ids`]). + pub(crate) fn into_direct(self) -> (Vec, HashSet) { + (self.direct, self.hoisted_peer_provider_node_ids) } /// Run the final per-importer peer pass and emit the result. Used diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs index ee3d2c281a..c97ce21e9e 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs @@ -137,6 +137,19 @@ pub struct ResolvePeersOptions { /// per-importer snapshots shared across every hoist-loop /// iteration. pub hoist_missing_scope: Option>, + + /// `NodeIds` of the peer providers the auto-install-peers loop + /// attached to importers' direct dependencies for reuse by other + /// subtrees. Each such node keeps its original position inside + /// the dependency tree, so the walk resolves its peers there; + /// walking it a second time as a root child would bind its peers + /// against the importer's own dependencies instead of the + /// providers next to it in the tree, and the root-context result + /// would overwrite the in-place one in `node_dep_paths`. The walk + /// therefore skips these direct entries unless the tree position + /// was pruned (an ancestor hit the peers cache) and nothing else + /// resolved them. + pub hoisted_peer_provider_node_ids: std::collections::HashSet, } /// See [`ResolvePeersOptions::hoist_missing_scope`]. @@ -184,6 +197,7 @@ impl Default for ResolvePeersOptions { lockfile_dir: None, modules_dir: None, hoist_missing_scope: None, + hoisted_peer_provider_node_ids: std::collections::HashSet::new(), } } } @@ -331,12 +345,35 @@ pub fn resolve_peers_workspace( let parent_node_ids: Vec = Vec::new(); let parent_pkg_ids_chain: Vec = Vec::new(); let importer_parent_dep_paths = walker.parent_dep_paths_from_refs(&importer_parents); - for dep in &importer.direct { + let (own_direct, provider_direct): (Vec<&DirectDep>, Vec<&DirectDep>) = importer + .direct + .iter() + .partition(|dep| !walker.opts.hoisted_peer_provider_node_ids.contains(&dep.node_id)); + for dep in &own_direct { walker .parent_pkgs_of_node .insert(dep.node_id.clone(), importer_parent_dep_paths.clone()); } - for dep in &importer.direct { + for dep in &own_direct { + walker.resolve_node( + dep.node_id.clone(), + &importer_parents, + &parent_chain_names, + &parent_node_ids, + &parent_pkg_ids_chain, + ); + } + // See ResolvePeersOptions::hoisted_peer_provider_node_ids — a + // provider is normally resolved at its tree position during the + // walk above; only one whose position was pruned still needs the + // root-context fallback. + for dep in &provider_direct { + if walker.node_dep_paths.contains_key(&dep.node_id) { + continue; + } + walker + .parent_pkgs_of_node + .insert(dep.node_id.clone(), importer_parent_dep_paths.clone()); walker.resolve_node( dep.node_id.clone(), &importer_parents, @@ -605,10 +642,33 @@ impl Walker<'_> { // `self.tree.direct`. let direct: Vec = self.tree.direct.clone(); let importer_parent_dep_paths = self.parent_dep_paths_from_refs(&importer_parents); - for dep in &direct { + let (own_direct, provider_direct): (Vec<&DirectDep>, Vec<&DirectDep>) = direct + .iter() + .partition(|dep| !self.opts.hoisted_peer_provider_node_ids.contains(&dep.node_id)); + for dep in &own_direct { self.parent_pkgs_of_node.insert(dep.node_id.clone(), importer_parent_dep_paths.clone()); } - for dep in &direct { + for dep in &own_direct { + let output = self.resolve_node( + dep.node_id.clone(), + &importer_parents, + &parent_chain_names, + &parent_node_ids, + &parent_pkg_ids_chain, + ); + for (peer_alias, peer_node_id) in output.auto_install_resolved_peers { + self.resolved_peer_providers_by_alias.insert(peer_alias, peer_node_id); + } + } + // See ResolvePeersOptions::hoisted_peer_provider_node_ids — a + // provider is normally resolved at its tree position during the + // walk above; only one whose position was pruned still needs the + // root-context fallback. + for dep in &provider_direct { + if self.node_dep_paths.contains_key(&dep.node_id) { + continue; + } + self.parent_pkgs_of_node.insert(dep.node_id.clone(), importer_parent_dep_paths.clone()); let output = self.resolve_node( dep.node_id.clone(), &importer_parents, diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs index 210dac3361..f3dfec1944 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs @@ -1,4 +1,7 @@ -use super::{NodeRecord, ResolvePeersOptions, Walker, resolve_peers, satisfies_with_prereleases}; +use super::{ + ImporterPeerInput, NodeRecord, ResolvePeersOptions, Walker, resolve_peers, + resolve_peers_workspace, satisfies_with_prereleases, +}; use crate::{ dependencies_graph::{DependenciesGraph, PeerDependencyIssues}, node_id::NodeId, @@ -1203,6 +1206,201 @@ fn shared_package_optional_transitive_peer_resolves_deterministically() { } } +/// A hoisted peer provider whose tree position was never visited (nothing in +/// the walk enumerates its node) must still be resolved by the root-context +/// fallback so consumers that bound it get a depPath. +#[test] +fn pruned_hoisted_provider_falls_back_to_root_resolution() { + let prov = NodeId::leaf("prov@1.0.0"); + let consumer = NodeId::next(); + + let mut tree = ResolvedTree { + direct: vec![ + DirectDep { + alias: "consumer".to_string(), + node_id: consumer.clone(), + id: "consumer@1.0.0".to_string(), + }, + DirectDep { + alias: "prov".to_string(), + node_id: prov.clone(), + id: "prov@1.0.0".to_string(), + }, + ], + packages: HashMap::from([ + ("prov@1.0.0".to_string(), package("prov", "1.0.0", &[], true)), + ("consumer@1.0.0".to_string(), package("consumer", "1.0.0", &[("prov", "*")], false)), + ]), + dependencies_tree: HashMap::from([ + (prov.clone(), tree_node("prov@1.0.0", BTreeMap::new(), 1)), + (consumer, tree_node("consumer@1.0.0", BTreeMap::new(), 0)), + ]), + all_peer_dep_names: HashSet::from(["prov".to_string()]), + policy_violations: Vec::new(), + applied_patches: HashSet::new(), + children_by_id: HashMap::new(), + }; + + let result = resolve_peers( + &mut tree, + ResolvePeersOptions { + hoisted_peer_provider_node_ids: HashSet::from([prov]), + ..ResolvePeersOptions::default() + }, + ); + + assert_eq!( + result.direct_dependencies_by_alias.get("prov"), + Some(&DepPath::from("prov@1.0.0")), + "the pruned provider must get a depPath from the fallback", + ); + assert!( + result.graph.contains_key(&DepPath::from("consumer@1.0.0(prov@1.0.0)")), + "the consumer must bind the fallback-resolved provider: {:#?}", + result.graph.keys().collect::>(), + ); +} + +/// Same as [`pruned_hoisted_provider_falls_back_to_root_resolution`] but +/// through the multi-importer entry point. +#[test] +fn pruned_hoisted_provider_falls_back_in_workspace_pass() { + let prov = NodeId::leaf("prov@1.0.0"); + let consumer = NodeId::next(); + + let importer = ImporterPeerInput { + id: ".".to_string(), + direct: vec![ + DirectDep { + alias: "consumer".to_string(), + node_id: consumer.clone(), + id: "consumer@1.0.0".to_string(), + }, + DirectDep { + alias: "prov".to_string(), + node_id: prov.clone(), + id: "prov@1.0.0".to_string(), + }, + ], + root_dir: std::path::PathBuf::from("/repo"), + modules_dir: None, + }; + let mut tree = ResolvedTree { + direct: Vec::new(), + packages: HashMap::from([ + ("prov@1.0.0".to_string(), package("prov", "1.0.0", &[], true)), + ("consumer@1.0.0".to_string(), package("consumer", "1.0.0", &[("prov", "*")], false)), + ]), + dependencies_tree: HashMap::from([ + (prov.clone(), tree_node("prov@1.0.0", BTreeMap::new(), 1)), + (consumer, tree_node("consumer@1.0.0", BTreeMap::new(), 0)), + ]), + all_peer_dep_names: HashSet::from(["prov".to_string()]), + policy_violations: Vec::new(), + applied_patches: HashSet::new(), + children_by_id: HashMap::new(), + }; + + let result = resolve_peers_workspace( + &mut tree, + &[importer], + std::path::Path::new("/repo"), + false, + false, + false, + ResolvePeersOptions { + hoisted_peer_provider_node_ids: HashSet::from([prov]), + ..ResolvePeersOptions::default() + }, + ); + + assert_eq!( + result.direct_dependencies_by_importer.get(".").and_then(|deps| deps.get("prov")), + Some(&DepPath::from("prov@1.0.0")), + "the pruned provider must get a depPath from the fallback", + ); + assert!( + result.graph.contains_key(&DepPath::from("consumer@1.0.0(prov@1.0.0)")), + "the consumer must bind the fallback-resolved provider: {:#?}", + result.graph.keys().collect::>(), + ); +} + +/// Mirror of the TS test "pruned hoisted peer providers that peer-depend on +/// each other are resolved together" (`deps-resolver/test/resolvePeers.ts`): +/// two pruned providers form a peer cycle, so each one's suffix depends on +/// the other's. Both must come out of the fallback with the cycle collapsed +/// to `name@version`, matching the in-place cycle handling. +#[test] +fn pruned_hoisted_providers_with_mutual_peers_resolve() { + let lib_a = NodeId::leaf("lib-a@1.0.0"); + let lib_b = NodeId::leaf("lib-b@1.0.0"); + let consumer = NodeId::next(); + + let mut tree = ResolvedTree { + direct: vec![ + DirectDep { + alias: "consumer".to_string(), + node_id: consumer.clone(), + id: "consumer@1.0.0".to_string(), + }, + DirectDep { + alias: "lib-a".to_string(), + node_id: lib_a.clone(), + id: "lib-a@1.0.0".to_string(), + }, + DirectDep { + alias: "lib-b".to_string(), + node_id: lib_b.clone(), + id: "lib-b@1.0.0".to_string(), + }, + ], + packages: HashMap::from([ + ("lib-a@1.0.0".to_string(), package("lib-a", "1.0.0", &[("lib-b", "^1.0.0")], true)), + ("lib-b@1.0.0".to_string(), package("lib-b", "1.0.0", &[("lib-a", "^1.0.0")], true)), + ( + "consumer@1.0.0".to_string(), + package("consumer", "1.0.0", &[("lib-a", "^1.0.0"), ("lib-b", "^1.0.0")], false), + ), + ]), + dependencies_tree: HashMap::from([ + (lib_a.clone(), tree_node("lib-a@1.0.0", BTreeMap::new(), 1)), + (lib_b.clone(), tree_node("lib-b@1.0.0", BTreeMap::new(), 1)), + (consumer, tree_node("consumer@1.0.0", BTreeMap::new(), 0)), + ]), + all_peer_dep_names: HashSet::from(["lib-a".to_string(), "lib-b".to_string()]), + policy_violations: Vec::new(), + applied_patches: HashSet::new(), + children_by_id: HashMap::new(), + }; + + let result = resolve_peers( + &mut tree, + ResolvePeersOptions { + hoisted_peer_provider_node_ids: HashSet::from([lib_a, lib_b]), + ..ResolvePeersOptions::default() + }, + ); + + assert_eq!( + result.direct_dependencies_by_alias.get("lib-a"), + Some(&DepPath::from("lib-a@1.0.0(lib-b@1.0.0)")), + "graph keys: {:#?}", + result.graph.keys().collect::>(), + ); + assert_eq!( + result.direct_dependencies_by_alias.get("lib-b"), + Some(&DepPath::from("lib-b@1.0.0(lib-a@1.0.0)")), + "graph keys: {:#?}", + result.graph.keys().collect::>(), + ); + assert!( + result.graph.contains_key(&DepPath::from("consumer@1.0.0(lib-a@1.0.0)(lib-b@1.0.0)")), + "the consumer must bind both fallback-resolved providers: {:#?}", + result.graph.keys().collect::>(), + ); +} + fn tree_node(pkg_id: &str, children: BTreeMap, depth: i32) -> DependenciesTreeNode { DependenciesTreeNode { resolved_package_id: pkg_id.to_string(), diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs index ffbe662017..f92d1d5609 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs @@ -253,12 +253,15 @@ where } } let mut per_importer_inputs: Vec = Vec::with_capacity(importers.len()); + let mut hoisted_peer_provider_node_ids = std::collections::HashSet::new(); for ((importer, state), (project_dir, modules_dir)) in importers.iter().zip(states).zip(input_dirs) { + let (direct, importer_provider_node_ids) = state.into_direct(); + hoisted_peer_provider_node_ids.extend(importer_provider_node_ids); per_importer_inputs.push(ImporterPeerInput { id: importer.id.clone(), - direct: state.into_direct(), + direct, root_dir: project_dir, modules_dir, }); @@ -284,6 +287,7 @@ where // importer's walk. modules_dir: None, hoist_missing_scope: None, + hoisted_peer_provider_node_ids, }; let peers = resolve_peers_workspace( &mut merged_tree, diff --git a/pacquet/crates/resolving-deps-resolver/src/tests.rs b/pacquet/crates/resolving-deps-resolver/src/tests.rs index 8e5cbfbde3..ddf5162d82 100644 --- a/pacquet/crates/resolving-deps-resolver/src/tests.rs +++ b/pacquet/crates/resolving-deps-resolver/src/tests.rs @@ -2152,7 +2152,7 @@ mod peers { exclude_links_from_lockfile: true, lockfile_dir: Some(lockfile_dir), modules_dir: Some(modules_dir), - hoist_missing_scope: None, + ..ResolvePeersOptions::default() }, ); diff --git a/pnpm11/__fixtures__/with-git-protocol-dep/pnpm-lock.yaml b/pnpm11/__fixtures__/with-git-protocol-dep/pnpm-lock.yaml index fcab7a6117..cd3fc01ec0 100644 --- a/pnpm11/__fixtures__/with-git-protocol-dep/pnpm-lock.yaml +++ b/pnpm11/__fixtures__/with-git-protocol-dep/pnpm-lock.yaml @@ -15,11 +15,10 @@ importers: packages: is-negative@https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c: - resolution: {tarball: https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c} + resolution: {gitHosted: true, tarball: https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c} version: 2.1.0 engines: {node: '>=0.10.0'} snapshots: - is-negative@https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c: - dev: false + is-negative@https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c: {} diff --git a/pnpm11/installing/deps-installer/test/install/autoInstallPeers.ts b/pnpm11/installing/deps-installer/test/install/autoInstallPeers.ts index add2697fd5..c2dc509664 100644 --- a/pnpm11/installing/deps-installer/test/install/autoInstallPeers.ts +++ b/pnpm11/installing/deps-installer/test/install/autoInstallPeers.ts @@ -758,3 +758,32 @@ test('a locked optional peer version is not rewritten when a sibling workspace p expect(optionalPeerVersion).not.toContain('(@pnpm.e2e/peer-c@1.0.0)') } }) + +test('a root dependency does not override the peers provided inside a self-contained subtree', async () => { + // Regression test for the closure-poisoning bug: @pnpm.e2e/closure-plugins + // provides every peer of its own subtree (closure-lib-a and closure-lib-b + // peer-depend on each other and on closure-peer-x, and all of them are + // regular dependencies of closure-plugins). The root project additionally + // depends on the incompatible closure-peer-x@2.0.0. With autoInstallPeers + // enabled, the peers resolved inside the subtree are also attached to the + // root project so other subtrees can reuse them — but they must not be + // peer-resolved again in the root context, where closure-peer-x@2.0.0 is + // the nearest provider, or the subtree's peer graph gets a mix of both + // versions. + const project = prepareEmpty() + await addDependenciesToPackage({}, [ + '@pnpm.e2e/closure-plugins@1.0.0', + '@pnpm.e2e/closure-peer-x@2.0.0', + ], testDefaults({ autoInstallPeers: true })) + const lockfile = project.readLockfile() + const pluginsSnapshot = lockfile.snapshots['@pnpm.e2e/closure-plugins@1.0.0'] + expect(pluginsSnapshot.dependencies?.['@pnpm.e2e/closure-peer-x']).toBe('1.0.0') + expect(pluginsSnapshot.dependencies?.['@pnpm.e2e/closure-lib-a']).not.toContain('closure-peer-x@2.0.0') + expect(pluginsSnapshot.dependencies?.['@pnpm.e2e/closure-lib-b']).not.toContain('closure-peer-x@2.0.0') + for (const depPath of Object.keys(lockfile.snapshots)) { + if (!depPath.startsWith('@pnpm.e2e/closure-lib')) continue + expect(depPath).not.toContain('closure-peer-x@2.0.0') + } + // The root keeps its own explicitly declared version. + expect(lockfile.importers['.'].dependencies?.['@pnpm.e2e/closure-peer-x']?.version).toBe('2.0.0') +}) diff --git a/pnpm11/installing/deps-resolver/src/index.ts b/pnpm11/installing/deps-resolver/src/index.ts index c78212b6c4..5cf5888235 100644 --- a/pnpm11/installing/deps-resolver/src/index.ts +++ b/pnpm11/installing/deps-resolver/src/index.ts @@ -77,6 +77,7 @@ interface ProjectToLink { binsDir: string declaredDirectDependencies: Set directNodeIdsByAlias: Map + hoistedPeerProviderNodeIds: Set explicitlyRequestedDirectDependencies: Set id: ProjectId linkedDependencies: LinkedDependency[] @@ -254,6 +255,7 @@ export async function resolveDependencies ( ...project.wantedDependencies.flatMap(({ alias, isNew }) => isNew && alias != null ? [alias] : []), ]), directNodeIdsByAlias: resolvedImporter.directNodeIdsByAlias, + hoistedPeerProviderNodeIds: resolvedImporter.hoistedPeerProviderNodeIds, explicitlyRequestedDirectDependencies: new Set( project.wantedDependencies.flatMap(({ alias, bareSpecifier, isNew, prevSpecifier, updateSpec }) => alias != null && (isNew === true || updateSpec === true || (prevSpecifier != null && bareSpecifier !== prevSpecifier)) diff --git a/pnpm11/installing/deps-resolver/src/resolveDependencies.ts b/pnpm11/installing/deps-resolver/src/resolveDependencies.ts index c5293238f8..1a8273d70e 100644 --- a/pnpm11/installing/deps-resolver/src/resolveDependencies.ts +++ b/pnpm11/installing/deps-resolver/src/resolveDependencies.ts @@ -281,6 +281,13 @@ export interface PkgAddress extends PkgAddressOrLinkBase { saveCatalogName?: string lockedPeerContext?: LockedPeerContext previousDepPath?: DepPath + /** + * A peer dependency provider attached to the root importer so that other + * subtrees can reuse it. Its `nodeId` keeps pointing at the provider's + * original position inside the dependency tree, so the node must be + * peer-resolved there — not in the root context. + */ + hoistedPeerProvider?: boolean } export type PkgAddressOrLink = PkgAddress | LinkedDependency @@ -431,7 +438,10 @@ export async function resolveRootDependencies ( // even those peers should be hoisted that are not autoinstalled for (const [resolvedPeerName, resolvedPeerAddress] of Object.entries(importerResolutionResult.resolvedPeers ?? {})) { if (!parentPkgAliases[resolvedPeerName]) { - importerResolutionResult.pkgAddresses.push(resolvedPeerAddress) + importerResolutionResult.pkgAddresses.push({ + ...resolvedPeerAddress, + hoistedPeerProvider: true, + }) } } } diff --git a/pnpm11/installing/deps-resolver/src/resolveDependencyTree.ts b/pnpm11/installing/deps-resolver/src/resolveDependencyTree.ts index ee0b291478..d78aeefe6b 100644 --- a/pnpm11/installing/deps-resolver/src/resolveDependencyTree.ts +++ b/pnpm11/installing/deps-resolver/src/resolveDependencyTree.ts @@ -47,6 +47,7 @@ export interface ResolvedImporters { [id: string]: { directDependencies: ResolvedDirectDependency[] directNodeIdsByAlias: Map + hoistedPeerProviderNodeIds: Set linkedDependencies: LinkedDependency[] } } @@ -357,6 +358,7 @@ export async function resolveDependencyTree ( } }), directNodeIdsByAlias: new Map(directNonLinkedDeps.map(({ alias, nodeId }) => [alias, nodeId])), + hoistedPeerProviderNodeIds: new Set(directNonLinkedDeps.filter((dep) => dep.hoistedPeerProvider).map(({ nodeId }) => nodeId)), linkedDependencies, } } diff --git a/pnpm11/installing/deps-resolver/src/resolvePeers.ts b/pnpm11/installing/deps-resolver/src/resolvePeers.ts index b015cee82e..ca3032f607 100644 --- a/pnpm11/installing/deps-resolver/src/resolvePeers.ts +++ b/pnpm11/installing/deps-resolver/src/resolvePeers.ts @@ -70,6 +70,8 @@ export interface GenericDependenciesGraphWithResolvedChildren + // See PkgAddress.hoistedPeerProvider in resolveDependencies.ts + hoistedPeerProviderNodeIds?: Set declaredDirectDependencies?: Set explicitlyRequestedDirectDependencies?: Set // only the top dependencies that were already installed @@ -121,7 +123,7 @@ export async function resolvePeers ( const finishingList: FinishingResolutionPromise[] = [] const peersCache = new Map() const purePkgs = new Set() - for (const { directNodeIdsByAlias, declaredDirectDependencies, explicitlyRequestedDirectDependencies, topParents, rootDir, id } of opts.projects) { + for (const { directNodeIdsByAlias, hoistedPeerProviderNodeIds, declaredDirectDependencies, explicitlyRequestedDirectDependencies, topParents, rootDir, id } of opts.projects) { const currentProviderSources: CurrentProviderSource[] = [{ directNodeIdsByAlias, declaredDirectDependencies: declaredDirectDependencies ?? new Set(), @@ -145,10 +147,25 @@ export async function resolvePeers ( } } - // eslint-disable-next-line no-await-in-loop - const { finishing } = await resolvePeersOfChildren(Object.fromEntries(directNodeIdsByAlias.entries()), pkgsByName, { + // Hoisted peer providers stay visible as providers (via pkgsByName) but are + // not traversed as direct children: their nodeIds point into subtrees, and + // resolving them a second time in the project's root context would bind + // their peers to the project's own dependencies instead of the providers + // next to them in the tree, racing with the in-place resolution on + // pathsByNodeId and producing peer graphs that mix both contexts. + const ownDirectChildren: Record = {} + const hoistedProviderChildren: Record = {} + for (const [alias, nodeId] of directNodeIdsByAlias.entries()) { + if (hoistedPeerProviderNodeIds?.has(nodeId)) { + hoistedProviderChildren[alias] = nodeId + } else { + ownDirectChildren[alias] = nodeId + } + } + const parentPkgsOfNode: ParentPkgsOfNode = new Map() + const projectPeersContext = { allPeerDepNames: opts.allPeerDepNames, - parentPkgsOfNode: new Map(), + parentPkgsOfNode, dependenciesTree: opts.dependenciesTree, depGraph, lockfileDir: opts.lockfileDir, @@ -168,10 +185,32 @@ export async function resolvePeers ( rootDir, virtualStoreDir: opts.virtualStoreDir, virtualStoreDirMaxLength: opts.virtualStoreDirMaxLength, - }) + } + // eslint-disable-next-line no-await-in-loop + const { finishing } = await resolvePeersOfChildren(ownDirectChildren, pkgsByName, projectPeersContext) if (finishing) { finishingList.push(finishing) } + // A provider whose tree position was pruned from the traversal (its parent + // hit peersCache, so its children were never visited) still has consumers + // awaiting its dep path, so resolve it here as a last resort. Providers + // visited by the traversal above are recorded in parentPkgsOfNode. + // All pruned providers go into a single resolvePeersOfChildren call: + // its cycle analysis only sees the children of one call, and providers + // frequently peer-depend on each other, so resolving them one by one + // would leave their dep path calculations awaiting each other forever. + const prunedProviderChildren: Record = {} + for (const [alias, nodeId] of Object.entries(hoistedProviderChildren)) { + if (parentPkgsOfNode.has(nodeId)) continue + prunedProviderChildren[alias] = nodeId + } + if (Object.keys(prunedProviderChildren).length > 0) { + // eslint-disable-next-line no-await-in-loop + const { finishing } = await resolvePeersOfChildren(prunedProviderChildren, pkgsByName, projectPeersContext) + if (finishing) { + finishingList.push(finishing) + } + } if (Object.keys(peerDependencyIssues.bad).length > 0 || Object.keys(peerDependencyIssues.missing).length > 0) { peerDependencyIssuesByProjects[id] = { ...peerDependencyIssues, diff --git a/pnpm11/installing/deps-resolver/test/resolvePeers.ts b/pnpm11/installing/deps-resolver/test/resolvePeers.ts index 154a3b77d8..9b15477f40 100644 --- a/pnpm11/installing/deps-resolver/test/resolvePeers.ts +++ b/pnpm11/installing/deps-resolver/test/resolvePeers.ts @@ -1717,3 +1717,147 @@ describe('dedupePeers', () => { expect(peerDependencyIssuesByProjects['']?.missing?.['real-pkg']).toBeUndefined() }) }) + +test('pruned hoisted peer providers that peer-depend on each other are resolved together', async () => { + // Hoisted peer providers whose tree position was never visited are resolved + // by a root-context fallback. Providers frequently peer-depend on each + // other, and each resolvePeersOfChildren call only detects peer cycles + // among its own children — so all pruned providers must be resolved in one + // call, or their dep path calculations await each other forever. + const libAPkg = { + name: 'lib-a', + pkgIdWithPatchHash: 'lib-a@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: { + 'lib-b': { version: '^1.0.0' }, + }, + id: '' as PkgResolutionId, + } + const libBPkg = { + name: 'lib-b', + pkgIdWithPatchHash: 'lib-b@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: { + 'lib-a': { version: '^1.0.0' }, + }, + id: '' as PkgResolutionId, + } + const consumerPkg = { + name: 'consumer', + pkgIdWithPatchHash: 'consumer@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: { + 'lib-a': { version: '^1.0.0' }, + 'lib-b': { version: '^1.0.0' }, + }, + id: '' as PkgResolutionId, + } + const { dependenciesGraph, dependenciesByProjectId } = await resolvePeers({ + allPeerDepNames: new Set(['lib-a', 'lib-b']), + projects: [ + { + directNodeIdsByAlias: new Map([ + ['consumer', '>consumer@1.0.0>' as NodeId], + ['lib-a', '>lib-a@1.0.0>' as NodeId], + ['lib-b', '>lib-b@1.0.0>' as NodeId], + ]), + hoistedPeerProviderNodeIds: new Set(['>lib-a@1.0.0>' as NodeId, '>lib-b@1.0.0>' as NodeId]), + topParents: [], + rootDir: '' as ProjectRootDir, + id: '.', + }, + ], + resolvedImporters: {}, + dependenciesTree: new Map>([ + ['>consumer@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: consumerPkg, + depth: 0, + }], + ['>lib-a@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: libAPkg, + depth: 1, + }], + ['>lib-b@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: libBPkg, + depth: 1, + }], + ]), + virtualStoreDir: '', + lockfileDir: '', + virtualStoreDirMaxLength: 120, + peersSuffixMaxLength: 1000, + workspaceProjectIds: new Set(), + }) + expect(Object.keys(dependenciesGraph).sort()).toStrictEqual([ + 'consumer@1.0.0(lib-a@1.0.0)(lib-b@1.0.0)', + 'lib-a@1.0.0(lib-b@1.0.0)', + 'lib-b@1.0.0(lib-a@1.0.0)', + ]) + expect(dependenciesByProjectId['.'].get('lib-a')).toBe('lib-a@1.0.0(lib-b@1.0.0)') + expect(dependenciesByProjectId['.'].get('lib-b')).toBe('lib-b@1.0.0(lib-a@1.0.0)') +}) + +test('a pruned hoisted peer provider is resolved by the root-context fallback', async () => { + // Mirror of the pacquet test `pruned_hoisted_provider_falls_back_to_root_resolution`: + // a hoisted peer provider whose tree position was never visited must still + // get a dep path so the consumers that bound it can finish. + const provPkg = { + name: 'prov', + pkgIdWithPatchHash: 'prov@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: {} as PeerDependencies, + id: '' as PkgResolutionId, + } + const consumerPkg = { + name: 'consumer', + pkgIdWithPatchHash: 'consumer@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: { + prov: { version: '*' }, + }, + id: '' as PkgResolutionId, + } + const { dependenciesGraph, dependenciesByProjectId } = await resolvePeers({ + allPeerDepNames: new Set(['prov']), + projects: [ + { + directNodeIdsByAlias: new Map([ + ['consumer', '>consumer@1.0.0>' as NodeId], + ['prov', '>prov@1.0.0>' as NodeId], + ]), + hoistedPeerProviderNodeIds: new Set(['>prov@1.0.0>' as NodeId]), + topParents: [], + rootDir: '' as ProjectRootDir, + id: '.', + }, + ], + resolvedImporters: {}, + dependenciesTree: new Map>([ + ['>consumer@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: consumerPkg, + depth: 0, + }], + ['>prov@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: provPkg, + depth: 1, + }], + ]), + virtualStoreDir: '', + lockfileDir: '', + virtualStoreDirMaxLength: 120, + peersSuffixMaxLength: 1000, + workspaceProjectIds: new Set(), + }) + expect(dependenciesByProjectId['.'].get('prov')).toBe('prov@1.0.0') + expect(Object.keys(dependenciesGraph)).toContain('consumer@1.0.0(prov@1.0.0)') +}) diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-a/1.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-a/1.0.0/package.json new file mode 100644 index 0000000000..605634df33 --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-a/1.0.0/package.json @@ -0,0 +1,8 @@ +{ + "name": "@pnpm.e2e/closure-lib-a", + "version": "1.0.0", + "peerDependencies": { + "@pnpm.e2e/closure-lib-b": "^1.0.0", + "@pnpm.e2e/closure-peer-x": "^1.0.0" + } +} diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-b/1.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-b/1.0.0/package.json new file mode 100644 index 0000000000..4c777eb10e --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-b/1.0.0/package.json @@ -0,0 +1,8 @@ +{ + "name": "@pnpm.e2e/closure-lib-b", + "version": "1.0.0", + "peerDependencies": { + "@pnpm.e2e/closure-lib-a": "^1.0.0", + "@pnpm.e2e/closure-peer-x": "^1.0.0" + } +} diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/1.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/1.0.0/package.json new file mode 100644 index 0000000000..da833c9c6b --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/1.0.0/package.json @@ -0,0 +1,4 @@ +{ + "name": "@pnpm.e2e/closure-peer-x", + "version": "1.0.0" +} diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/2.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/2.0.0/package.json new file mode 100644 index 0000000000..85535e2d1c --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/2.0.0/package.json @@ -0,0 +1,4 @@ +{ + "name": "@pnpm.e2e/closure-peer-x", + "version": "2.0.0" +} diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-plugins/1.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-plugins/1.0.0/package.json new file mode 100644 index 0000000000..addcf13e29 --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-plugins/1.0.0/package.json @@ -0,0 +1,9 @@ +{ + "name": "@pnpm.e2e/closure-plugins", + "version": "1.0.0", + "dependencies": { + "@pnpm.e2e/closure-lib-a": "1.0.0", + "@pnpm.e2e/closure-lib-b": "1.0.0", + "@pnpm.e2e/closure-peer-x": "1.0.0" + } +}