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()),