From 76fa59584ee4e1bacef4fd930de847f76f7d5ec4 Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Mon, 26 Oct 2026 22:22:38 +0100 Subject: [PATCH] fix(hoist): preserve peer variants in workspaces (#17397) With nodeLinker: hoisted and autoInstallPeers: false, a package that two workspace projects use with different peer versions got only one copy, so the second project resolved the first project's peer. pnpm v11 nests the second variant; this was a v12-only regression. The hoister now keeps a peer-bearing node's own snapshot key as its reference. A node without peer names still collapses onto the first-seen variant, as pnpm v11's depPathByPkgId does, which keeps peer-variant-heavy lockfiles from exploding the per-path walk. When autoInstallPeers is on, peer names are empty and every variant collapses as before. The hoisted dep-graph walk indexes package locations by exact snapshot key as well as by package id. Importer direct dependencies and child edges resolve to their own variant's copy first, then fall back to the package id for variants the hoister collapsed. Keeping the collapse and adding yarn's peer check to the hoister's dedup path also places the copies correctly, but the walker then sees two copies with one snapshot key. The hoisted linker treats those as duplicates, builds one, and symlinks the other, which cross-links the variants again. On a large real lockfile with autoInstallPeers off, the hoisted tree grows from 8358 to 17157 nodes. The yarn-parity alternative produces 17570, so the growth is the copies peer resolution requires. Closes pnpm/pnpm#17395 --------- Co-authored-by: Zoltan Kochan Co-authored-by: Claude Opus 5.5 --- .changeset/quiet-paws-hoist.md | 5 ++ .../cli/tests/suite/hoisted_node_linker.rs | 28 +++++++ .../deps-restorer/src/hoisted_dep_graph.rs | 4 +- .../src/hoisted_dep_graph/walk.rs | 80 ++++++++++++------- pnpm/crates/real-hoist/src/tree.rs | 42 +++++----- 5 files changed, 108 insertions(+), 51 deletions(-) create mode 100644 .changeset/quiet-paws-hoist.md diff --git a/.changeset/quiet-paws-hoist.md b/.changeset/quiet-paws-hoist.md new file mode 100644 index 0000000000..7fa00a0c27 --- /dev/null +++ b/.changeset/quiet-paws-hoist.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +With `nodeLinker: hoisted` and `autoInstallPeers: false`, a package that workspace projects use with different peer versions is now installed once per peer variant, so each project's copy resolves that project's peer [#17395](https://github.com/pnpm/pnpm/issues/17395). diff --git a/pnpm/crates/cli/tests/suite/hoisted_node_linker.rs b/pnpm/crates/cli/tests/suite/hoisted_node_linker.rs index 56d2b7627c..42f5329476 100644 --- a/pnpm/crates/cli/tests/suite/hoisted_node_linker.rs +++ b/pnpm/crates/cli/tests/suite/hoisted_node_linker.rs @@ -1173,6 +1173,34 @@ fn peer_variants_of_one_version_share_the_root_slot() { } } +#[test] +fn peer_variants_with_distinct_peers_are_nested_for_node_resolution() { + let fixture = WorkspaceFixture::new(); + fixture.append_workspace_yaml("nodeLinker: hoisted\nautoInstallPeers: false\n"); + let deps_with_peer_a_1_0_0 = + [("@pnpm.e2e/abc-optional-peers-meta-only", "1.0.0"), ("@pnpm.e2e/peer-a", "1.0.0")]; + let deps_with_peer_a_1_0_1 = + [("@pnpm.e2e/abc-optional-peers-meta-only", "1.0.0"), ("@pnpm.e2e/peer-a", "1.0.1")]; + fixture.project("a", "a", ManifestDeps { prod: &deps_with_peer_a_1_0_0, ..Default::default() }); + fixture.project("b", "b", ManifestDeps { prod: &deps_with_peer_a_1_0_1, ..Default::default() }); + + fixture.run(["install"]); + + let output = Command::new("node") + .arg("-e") + .arg( + r"const path = require('node:path'); const fs = require('node:fs'); for (const project of ['a', 'b']) { const projectDir = path.resolve('packages', project); const abcDir = path.dirname(require.resolve('@pnpm.e2e/abc-optional-peers-meta-only/package.json', { paths: [projectDir] })); const peerManifest = require.resolve('@pnpm.e2e/peer-a/package.json', { paths: [abcDir] }); process.stdout.write(`${project}:${JSON.parse(fs.readFileSync(peerManifest, 'utf8')).version}\n`); }", + ) + .current_dir(&fixture.workspace) + .output() + .expect("run Node.js module resolution"); + assert!(output.status.success(), "Node.js resolution failed: {output:?}"); + assert_eq!( + String::from_utf8(output.stdout).expect("Node.js output is UTF-8"), + "a:1.0.0\nb:1.0.1\n", + ); +} + #[cfg(unix)] mod local_symlinks; diff --git a/pnpm/crates/deps-restorer/src/hoisted_dep_graph.rs b/pnpm/crates/deps-restorer/src/hoisted_dep_graph.rs index b6c1ffbe64..7edb8617ef 100644 --- a/pnpm/crates/deps-restorer/src/hoisted_dep_graph.rs +++ b/pnpm/crates/deps-restorer/src/hoisted_dep_graph.rs @@ -22,7 +22,7 @@ use presence::{ }; mod walk; -use walk::{WalkState, walk_deps}; +use walk::{PackageLocations, WalkState, walk_deps}; use crate::{HoistedLocations, safe_join_modules_dir::InvalidDependencyAliasError}; use derive_more::{Display, Error, From}; @@ -329,7 +329,7 @@ fn build_dep_graph<'a>( opts, prev_graph, - pkg_locations_by_pkg_id: BTreeMap::new(), + package_locations: PackageLocations::default(), per_importer_hierarchies: BTreeMap::new(), per_importer_direct_deps: BTreeMap::new(), diff --git a/pnpm/crates/deps-restorer/src/hoisted_dep_graph/walk.rs b/pnpm/crates/deps-restorer/src/hoisted_dep_graph/walk.rs index 70d7e59e2b..3ed640a103 100644 --- a/pnpm/crates/deps-restorer/src/hoisted_dep_graph/walk.rs +++ b/pnpm/crates/deps-restorer/src/hoisted_dep_graph/walk.rs @@ -11,17 +11,56 @@ use pnpm_lockfile::{Lockfile, LockfileResolution, PackageKey, PkgIdWithPatchHash use pnpm_modules_yaml::DepPath; use pnpm_real_hoist::{HoisterResult, RcByPtr}; use std::{ - collections::BTreeMap, + collections::{BTreeMap, HashMap}, path::{Path, PathBuf}, }; +/// Every directory each package landed in, in visit order. The first +/// entry wins for parent → child wiring. +/// +/// A lookup tries the exact snapshot key, then falls back to +/// [`pnpm_real_hoist::pkg_id`]. A peer variant the hoister collapsed +/// reaches the walk under the first-seen variant's key, so an edge +/// declared against any other variant finds that node through the +/// package id. A variant the hoister kept distinct is found by its own +/// key. +#[derive(Default)] +pub(super) struct PackageLocations { + by_pkg_id: BTreeMap>, + by_package_key: HashMap>, +} + +impl PackageLocations { + fn first(&self, pkg_key: &PackageKey) -> Option<&PathBuf> { + self.by_package_key + .get(pkg_key) + .and_then(|locations| locations.first()) + .or_else(|| { + self.by_pkg_id + .get(&pnpm_real_hoist::pkg_id(pkg_key)) + .and_then(|locations| locations.first()) + }) + } + + fn record(&mut self, pkg_key: &PackageKey, dir: &Path) { + self.by_pkg_id + .entry(pnpm_real_hoist::pkg_id(pkg_key)) + .or_default() + .push(dir.to_path_buf()); + self.by_package_key + .entry(pkg_key.clone()) + .or_default() + .push(dir.to_path_buf()); + } +} + impl WalkState<'_> { /// Pass 2 — fill in each node's `children` map from the - /// now-complete `pkg_locations_by_pkg_id`, then assemble the result. + /// now-complete package location index, then assemble the result. /// /// The walk intentionally leaves `children` empty: every sibling and /// descendant of a node must have its directory recorded in - /// `pkg_locations_by_pkg_id` before any node resolves its children, + /// package locations before any node resolves its children, /// so the location index is complete by the time children are /// computed. The simplest way to preserve that invariant is to /// insert everything first and resolve children second. @@ -29,7 +68,7 @@ impl WalkState<'_> { mut self, root_hierarchy: DepHierarchy, ) -> Result { - fill_children(&mut self.result.graph, &self.pkg_locations_by_pkg_id, self.lockfile)?; + fill_children(&mut self.result.graph, &self.package_locations, self.lockfile)?; // The hoister produced a children order; the directory keys in // `root_hierarchy` follow it, and @@ -50,7 +89,7 @@ impl WalkState<'_> { let Some(importer) = self.lockfile.importers.get(importer_id) else { continue }; direct_dependencies_by_importer_id.insert( importer_id.clone(), - importer_direct_deps(importer, &self.pkg_locations_by_pkg_id), + importer_direct_deps(importer, &self.package_locations), ); } @@ -90,7 +129,7 @@ pub(super) fn root_direct_deps( /// recorded location of each resolved snapshot wins. pub(super) fn importer_direct_deps( importer: &pnpm_lockfile::ProjectSnapshot, - pkg_locations_by_pkg_id: &BTreeMap>, + pkg_locations: &PackageLocations, ) -> BTreeMap { let mut direct_deps = BTreeMap::new(); for dep_map in [ @@ -105,10 +144,7 @@ pub(super) fn importer_direct_deps( // For an aliased dep the snapshot key uses the alias's own // (name, suffix); for a regular dep it's `(alias, version)`. let Some(dep_key) = spec.version.resolved_key(alias) else { continue }; - if let Some(first) = pkg_locations_by_pkg_id - .get(&pnpm_real_hoist::pkg_id(&dep_key)) - .and_then(|locations| locations.first()) - { + if let Some(first) = pkg_locations.first(&dep_key) { direct_deps.insert(alias.to_string(), first.clone()); } } @@ -120,7 +156,7 @@ pub(super) fn importer_direct_deps( /// dir` map by looking up the node's snapshot in the lockfile. pub(super) fn fill_children( graph: &mut DependenciesGraph, - pkg_locations: &BTreeMap>, + pkg_locations: &PackageLocations, lockfile: &Lockfile, ) -> Result<(), HoistedDepGraphError> { let dirs: Vec = graph.keys().cloned().collect(); @@ -158,16 +194,7 @@ pub(super) struct WalkState<'a> { /// way as the one going there now. `None` on the fresh-lockfile /// path, which has no current lockfile to walk. pub(super) prev_graph: Option<&'a DependenciesGraph>, - /// Records every directory each package landed in, in visit - /// order. The first entry wins for parent → child wiring. - /// - /// Keyed by [`pnpm_real_hoist::pkg_id`], not the snapshot key: - /// the hoister collapses every peer variant of one package - /// version onto a single node, so only the first-seen variant's - /// key reaches this walk. Sharing the hoister's own identity - /// function is what lets an edge declared against any other - /// variant still find that node's directory. - pub(super) pkg_locations_by_pkg_id: BTreeMap>, + pub(super) package_locations: PackageLocations, /// Per-non-root-importer hierarchy emitted while walking /// `Workspace`-kind nodes. Outer key is the importer's root /// directory (`/`). Folded into @@ -433,7 +460,7 @@ pub(super) fn graph_node( /// location. pub(super) fn compute_children( snapshot: Option<&pnpm_lockfile::SnapshotEntry>, - pkg_locations: &BTreeMap>, + pkg_locations: &PackageLocations, ) -> BTreeMap { let mut children: BTreeMap = BTreeMap::new(); let Some(snapshot) = snapshot else { return children }; @@ -448,9 +475,7 @@ pub(super) fn compute_children( let Some(child_key) = dep_ref.resolve(alias_name) else { continue; }; - if let Some(locations) = pkg_locations.get(&pnpm_real_hoist::pkg_id(&child_key)) - && let Some(first) = locations.first() - { + if let Some(first) = pkg_locations.first(&child_key) { children.insert(alias_name.to_string(), first.clone()); } } @@ -458,8 +483,5 @@ pub(super) fn compute_children( } fn record_package_location(state: &mut WalkState<'_>, pkg_key: &PackageKey, dir: &Path) { - state.pkg_locations_by_pkg_id - .entry(pnpm_real_hoist::pkg_id(pkg_key)) - .or_default() - .push(dir.to_path_buf()); + state.package_locations.record(pkg_key, dir); } diff --git a/pnpm/crates/real-hoist/src/tree.rs b/pnpm/crates/real-hoist/src/tree.rs index 98dc43ca5e..1884c3d365 100644 --- a/pnpm/crates/real-hoist/src/tree.rs +++ b/pnpm/crates/real-hoist/src/tree.rs @@ -64,14 +64,17 @@ pub(super) fn importer_node( /// Conversion-phase caches. `nodes` interns one [`HoisterTree`] per /// `(alias, snapshot key)` edge target. `dep_key_by_pkg_id` maps a -/// package id (see [`pkg_id`]) to the first snapshot key seen for -/// it: every peer-suffix variant of one package version -/// gets that first key as its `reference`, so the hoister sees one -/// locator per version and dedups the variants instead of -/// conflict-nesting a copy of each. Ports `depPathByPkgId` from pnpm -/// v11's `@pnpm/real-hoist` wrapper — without the collapse, a -/// peer-variant-heavy lockfile turns the per-path hoist walk into a -/// combinatorial explosion of nested conflict copies. +/// package id (see [`pkg_id`]) to the first snapshot key seen for it. +/// +/// A node without peer names gets that first key as its `reference`, +/// so the hoister sees one locator per package version and dedups the +/// peer variants instead of nesting a copy of each. Its children still +/// come from its own snapshot. Ports `depPathByPkgId` from pnpm v11's +/// `@pnpm/real-hoist` wrapper. +/// +/// A node with peer names keeps its own snapshot key. The hoister then +/// nests the variant wherever its peers resolve differently, and the +/// walker knows which snapshot each copy holds. #[derive(Default)] pub(super) struct TreeCache { pub(super) nodes: HashMap>, @@ -161,22 +164,21 @@ fn build_dep_node( // outer call returns the cell holds the populated set, and the // shared-by-identity invariant the hoister algorithm relies on // survives. - // The reference is the *canonical* snapshot key for this package - // version — the first-seen peer-suffix variant — not necessarily - // `dep_key` itself (see [`TreeCache::dep_key_by_pkg_id`]). The - // node's own children still come from `dep_key`'s snapshot, - // matching the TS wrapper, which reads `pkgSnapshot` from the - // original depPath while stamping `depPathByPkgId.get(id)` as the - // reference. - let reference = cache.dep_key_by_pkg_id - .entry(pkg_id(dep_key)) - .or_insert_with(|| dep_key.clone()) - .to_string(); + let peer_names = peer_names_of(dep_key, snapshot, lockfile, opts); + // See [`TreeCache`] for when a variant keeps its own key. + let reference = if peer_names.is_empty() { + cache.dep_key_by_pkg_id + .entry(pkg_id(dep_key)) + .or_insert_with(|| dep_key.clone()) + .to_string() + } else { + dep_key.to_string() + }; let node = Rc::new(HoisterTree { name: alias.to_string(), ident_name: dep_key.name.to_string(), reference, - peer_names: peer_names_of(dep_key, snapshot, lockfile, opts), + peer_names, dependency_kind: HoisterDependencyKind::Regular, hoist_priority: 0, dependencies: RefCell::new(IndexSet::new()),