fix(hoisted): keep injected peer variants as separate copies (#14126)
Under `nodeLinker: hoisted`, peer-resolution variants of an **injected directory dependency** (a `file:` snapshot) collapsed onto the first-seen variant, like registry peer variants do since #14039 (and the TypeScript CLI's long-standing `depPathByPkgId` mapping). With #14034, edges declared against a losing variant then resolve to the surviving copy. For registry packages that dedup is intended: every variant unpacks the same tarball, and peers resolve through the ancestry either way. An injected directory dep is different — each `file:` variant is materialized as its **own copy of the local package with its own peer-resolved dependency set**; that is the point of injecting. Collapsing rewires every dependent of the losing variant onto the survivor's children. ### Symptom teambit/bit's root-components e2e (`nodeLinker: hoisted`): two Bit root components pin `is-odd@1` and `is-odd@2` across injected copies of the same component (`comp2@file:comp2(is-odd@1.0.0)` / `(is-odd@2.0.0)`). The second root's copy materialized as a symlink into the first root's tree and resolved the wrong peer: ``` AssertionError: expected '1.0.0' to match /^2\./ ``` ### The fix Exempt `file:` snapshots from the collapse in the shared identity function — `pkg_id` in pacquet, `getHoisterPkgId` in the TypeScript CLI. Every index over the hoist result (`pkg_locations_by_pkg_id`, `package_ids_by_pkg_id`, `pkgLocationsByPkgId`) derives its keys from that function, so the variants stay apart with no further changes. The registry collapse stays untouched: it is what stopped the per-path walk from exploding on peer-variant-heavy lockfiles (the OOM #14039 fixed), and `file:` snapshots cannot explode that way — there is one per injected workspace package.
This commit is contained in:
1 parent
afb4071811
commit
2f703cb8d7
7 files changed
+319
-5
No files matched your search
@@ -0,0 +1,8 @@
|
||||
---
|
||||
"@pnpm/installing.deps-restorer": patch
|
||||
"@pnpm/installing.linking.real-hoist": patch
|
||||
"pacquet": patch
|
||||
"pnpm": patch
|
||||
---
|
||||
|
||||
Under `nodeLinker: hoisted`, peer-resolution variants of an injected directory dependency (a `file:` snapshot) are materialized as separate copies again instead of collapsing onto the first-seen variant. Each copy keeps its own peer-resolved dependency set, so a project pinning one peer version no longer resolves another project's variant — Bit root components with conflicting peers across injected copies rely on this.
|
||||
@@ -1175,3 +1175,88 @@ fn walker_wires_edges_declared_against_a_collapsed_peer_variant() {
|
||||
"a snapshot edge on the collapsed variant resolves to the surviving copy",
|
||||
);
|
||||
}
|
||||
|
||||
/// Peer variants of an injected directory dependency are exempt from
|
||||
/// the collapse (see [`pnpm_real_hoist::pkg_id`]), so the walk has to
|
||||
/// keep a location — and a direct-dependency entry — per variant,
|
||||
/// where every collapsed package funnels into one.
|
||||
#[test]
|
||||
fn walker_keeps_file_dep_peer_variants_apart() {
|
||||
let mut r1_deps = ResolvedDependencyMap::new();
|
||||
r1_deps.insert(
|
||||
pkg_name("comp"),
|
||||
ResolvedDependencySpec {
|
||||
specifier: "workspace:*".to_string(),
|
||||
version: ver_peer("file:comp(peer@1.0.0)").into(),
|
||||
},
|
||||
);
|
||||
r1_deps.insert(pkg_name("peer"), resolved_dep("1.0.0"));
|
||||
let mut r2_deps = ResolvedDependencyMap::new();
|
||||
r2_deps.insert(
|
||||
pkg_name("comp"),
|
||||
ResolvedDependencySpec {
|
||||
specifier: "workspace:*".to_string(),
|
||||
version: ver_peer("file:comp(peer@2.0.0)").into(),
|
||||
},
|
||||
);
|
||||
r2_deps.insert(pkg_name("peer"), resolved_dep("2.0.0"));
|
||||
|
||||
let mut packages = HashMap::new();
|
||||
packages.insert(
|
||||
dep_key("comp", "file:comp"),
|
||||
PackageMetadata { resolution: directory_resolution("comp"), ..metadata_stub() },
|
||||
);
|
||||
packages.insert(dep_key("peer", "1.0.0"), metadata_stub());
|
||||
packages.insert(dep_key("peer", "2.0.0"), metadata_stub());
|
||||
|
||||
let mut snapshots = HashMap::new();
|
||||
for peer_version in ["1.0.0", "2.0.0"] {
|
||||
let mut comp_deps = HashMap::new();
|
||||
comp_deps.insert(pkg_name("peer"), SnapshotDepRef::Plain(ver_peer(peer_version)));
|
||||
snapshots.insert(
|
||||
dep_key("comp", &format!("file:comp(peer@{peer_version})")),
|
||||
SnapshotEntry { dependencies: Some(comp_deps), ..SnapshotEntry::default() },
|
||||
);
|
||||
snapshots.insert(dep_key("peer", peer_version), SnapshotEntry::default());
|
||||
}
|
||||
|
||||
let lockfile = workspace_lockfile(
|
||||
vec![
|
||||
(Lockfile::ROOT_IMPORTER_KEY, ResolvedDependencyMap::new()),
|
||||
("node_modules/.bit_roots/r1", r1_deps),
|
||||
("node_modules/.bit_roots/r2", r2_deps),
|
||||
],
|
||||
packages,
|
||||
snapshots,
|
||||
);
|
||||
let opts = LockfileToHoistedDepGraphOptions {
|
||||
lockfile_dir: PathBuf::from("/repo"),
|
||||
..LockfileToHoistedDepGraphOptions::default()
|
||||
};
|
||||
let result = lockfile_to_hoisted_dep_graph(&lockfile, None, &opts).expect("walker succeeds");
|
||||
|
||||
let r1_comp = result.direct_dependencies_by_importer_id["node_modules/.bit_roots/r1"]
|
||||
.get("comp")
|
||||
.expect("r1 keeps its comp direct dependency")
|
||||
.clone();
|
||||
let r2_comp = result.direct_dependencies_by_importer_id["node_modules/.bit_roots/r2"]
|
||||
.get("comp")
|
||||
.expect("r2 keeps its comp direct dependency")
|
||||
.clone();
|
||||
assert_ne!(
|
||||
r1_comp, r2_comp,
|
||||
"each importer's direct dependency must be its own variant's copy",
|
||||
);
|
||||
let r1_peer = result.graph[&r1_comp].children.get("peer").expect("r1's copy resolves peer");
|
||||
let r2_peer = result.graph[&r2_comp].children.get("peer").expect("r2's copy resolves peer");
|
||||
assert_eq!(
|
||||
result.graph[r1_peer].dep_path,
|
||||
DepPath::from("peer@1.0.0".to_string()),
|
||||
"r1's copy must resolve the peer version r1 pinned",
|
||||
);
|
||||
assert_eq!(
|
||||
result.graph[r2_peer].dep_path,
|
||||
DepPath::from("peer@2.0.0".to_string()),
|
||||
"r2's copy must resolve the peer version r2 pinned",
|
||||
);
|
||||
}
|
||||
@@ -297,9 +297,12 @@ impl Lockfile {
|
||||
}
|
||||
|
||||
/// Whether `path` ends in a tarball extension (`.tgz`, `.tar.gz`, or
|
||||
/// `.tar`, case-insensitively), so the directory-vs-tarball boundary
|
||||
/// applied here matches the resolver's at resolve time.
|
||||
fn is_local_tarball_path(path: &str) -> bool {
|
||||
/// `.tar`, case-insensitively) — the directory-vs-tarball boundary the
|
||||
/// resolver applies to a `file:` spec at resolve time. Public so
|
||||
/// consumers classifying a `file:` snapshot key (such as the hoister's
|
||||
/// identity function) draw the same line.
|
||||
#[must_use]
|
||||
pub fn is_local_tarball_path(path: &str) -> bool {
|
||||
let lower = path.as_bytes();
|
||||
let ends_with_ci = |suffix: &str| {
|
||||
let bytes = suffix.as_bytes();
|
||||
|
||||
@@ -40,7 +40,9 @@
|
||||
use derive_more::{Display, Error};
|
||||
use indexmap::{IndexMap, IndexSet};
|
||||
use miette::Diagnostic;
|
||||
use pnpm_lockfile::{Lockfile, PkgName, PkgNameVerPeer, ProjectSnapshot, SnapshotEntry};
|
||||
use pnpm_lockfile::{
|
||||
Lockfile, PkgName, PkgNameVerPeer, ProjectSnapshot, SnapshotEntry, VersionPart,
|
||||
};
|
||||
use std::{
|
||||
cell::{Cell, RefCell},
|
||||
collections::{BTreeMap, BTreeSet, HashMap, HashSet, VecDeque},
|
||||
@@ -605,8 +607,27 @@ fn collect_snapshot_deps(
|
||||
/// package under a second name gets a node, and a directory, of its
|
||||
/// own. An index over the result holds the list and resolves an edge
|
||||
/// to its first entry.
|
||||
///
|
||||
/// An injected directory dependency (a `file:` version that is not a
|
||||
/// local tarball) keeps its peer suffix: every variant of it is a
|
||||
/// separate on-disk copy of the local package, materialized with its
|
||||
/// own peer-resolved dependency set, so collapsing the variants would
|
||||
/// rewire every dependent of the losing one onto the survivor's
|
||||
/// children (Bit's root components pin conflicting peers across such
|
||||
/// copies on purpose). The registry collapse exists to stop
|
||||
/// peer-variant explosion on large lockfiles; directory snapshots are
|
||||
/// one per injected workspace package and cannot explode that way. A
|
||||
/// local tarball (`file:foo.tgz`) unpacks the same archive for every
|
||||
/// variant like a registry package, so it collapses like one — the
|
||||
/// boundary is [`pnpm_lockfile::is_local_tarball_path`], the same one
|
||||
/// the lockfile itself draws for `file:` resolutions.
|
||||
#[must_use]
|
||||
pub fn pkg_id(dep_key: &PkgNameVerPeer) -> String {
|
||||
if let VersionPart::File(path) = dep_key.suffix.version()
|
||||
&& !pnpm_lockfile::is_local_tarball_path(path)
|
||||
{
|
||||
return dep_key.to_string();
|
||||
}
|
||||
dep_key.without_peer().to_string()
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use super::{
|
||||
HoistError, HoistOpts, HoisterResult, RcByPtr, build_hoist_ident_map, hoist, is_preferred_ident,
|
||||
HoistError, HoistOpts, HoisterResult, RcByPtr, build_hoist_ident_map, hoist,
|
||||
is_preferred_ident, percent_encode_path,
|
||||
};
|
||||
use indexmap::IndexSet;
|
||||
use pnpm_lockfile::{
|
||||
@@ -1316,6 +1317,131 @@ fn peer_suffix_variants_collapse_to_one_hoisted_copy() {
|
||||
);
|
||||
}
|
||||
|
||||
/// Peer variants of an injected directory dependency are exempt from
|
||||
/// the collapse — see [`pkg_id`] for why. The fixture mirrors the
|
||||
/// teambit/bit root-components layout that caught the regression: two
|
||||
/// importers on the same `file:` package, pinning conflicting peers.
|
||||
#[test]
|
||||
fn file_dep_peer_variants_keep_their_own_copies() {
|
||||
let mut importers = HashMap::new();
|
||||
importers.insert(Lockfile::ROOT_IMPORTER_KEY.to_string(), ProjectSnapshot::default());
|
||||
for (importer_id, peer_ver) in
|
||||
[("node_modules/.bit_roots/r1", "1.0.0"), ("node_modules/.bit_roots/r2", "2.0.0")]
|
||||
{
|
||||
let mut deps = ResolvedDependencyMap::new();
|
||||
deps.insert(
|
||||
pkg_name("comp"),
|
||||
ResolvedDependencySpec {
|
||||
specifier: "workspace:*".to_string(),
|
||||
version: ver_peer(&format!("file:comp(p@{peer_ver})")).into(),
|
||||
},
|
||||
);
|
||||
deps.insert(pkg_name("p"), resolved_dep(peer_ver));
|
||||
importers.insert(
|
||||
importer_id.to_string(),
|
||||
ProjectSnapshot { dependencies: Some(deps), ..ProjectSnapshot::default() },
|
||||
);
|
||||
}
|
||||
|
||||
let mut snapshots = HashMap::new();
|
||||
for peer_ver in ["1.0.0", "2.0.0"] {
|
||||
let mut comp_deps = HashMap::new();
|
||||
comp_deps.insert(pkg_name("p"), SnapshotDepRef::Plain(ver_peer(peer_ver)));
|
||||
snapshots.insert(
|
||||
dep_key("comp", &format!("file:comp(p@{peer_ver})")),
|
||||
SnapshotEntry { dependencies: Some(comp_deps), ..SnapshotEntry::default() },
|
||||
);
|
||||
snapshots.insert(dep_key("p", peer_ver), SnapshotEntry::default());
|
||||
}
|
||||
|
||||
let lockfile = Lockfile { importers, snapshots: Some(snapshots), ..empty_lockfile() };
|
||||
|
||||
let result =
|
||||
hoist(&lockfile, &HoistOpts::default()).expect("file-variant hoist should succeed");
|
||||
|
||||
// Node resolution walks up from the importer, so the copy an
|
||||
// importer sees is the one nested in its own subtree — or the
|
||||
// root's, when its variant was hoisted there. Either placement is
|
||||
// correct only if the reference reached this way is the variant
|
||||
// the importer declared.
|
||||
let root_children = result.dependencies.borrow();
|
||||
let comp_reference_seen_by = |importer: &str| -> String {
|
||||
let importer_name = percent_encode_path(importer);
|
||||
let importer_node =
|
||||
&root_children.iter().find(|dep| dep.0.name == importer_name).unwrap().0;
|
||||
let importer_children = importer_node.dependencies.borrow();
|
||||
let comp = importer_children
|
||||
.iter()
|
||||
.find(|dep| dep.0.name == "comp")
|
||||
.or_else(|| root_children.iter().find(|dep| dep.0.name == "comp"))
|
||||
.unwrap_or_else(|| panic!("no comp reachable from {importer}"));
|
||||
comp.0.references.borrow().iter().next().cloned().unwrap_or_default()
|
||||
};
|
||||
assert_eq!(
|
||||
comp_reference_seen_by("node_modules/.bit_roots/r1"),
|
||||
"comp@file:comp(p@1.0.0)",
|
||||
"r1 must resolve the copy carrying its own peer variant",
|
||||
);
|
||||
assert_eq!(
|
||||
comp_reference_seen_by("node_modules/.bit_roots/r2"),
|
||||
"comp@file:comp(p@2.0.0)",
|
||||
"r2 must resolve the copy carrying its own peer variant",
|
||||
);
|
||||
}
|
||||
|
||||
/// The directory exemption in [`pkg_id`] must not widen to local
|
||||
/// tarballs: a `file:*.tgz` dependency collapses its peer variants
|
||||
/// like a registry package.
|
||||
#[test]
|
||||
fn file_tarball_peer_variants_collapse_like_registry_packages() {
|
||||
let mut importers = HashMap::new();
|
||||
importers.insert(Lockfile::ROOT_IMPORTER_KEY.to_string(), ProjectSnapshot::default());
|
||||
for (importer_id, peer_ver) in [("packages/a", "1.0.0"), ("packages/b", "2.0.0")] {
|
||||
let mut deps = ResolvedDependencyMap::new();
|
||||
deps.insert(
|
||||
pkg_name("tarpkg"),
|
||||
ResolvedDependencySpec {
|
||||
specifier: "file:tarpkg.tgz".to_string(),
|
||||
version: ver_peer(&format!("file:tarpkg.tgz(p@{peer_ver})")).into(),
|
||||
},
|
||||
);
|
||||
deps.insert(pkg_name("p"), resolved_dep(peer_ver));
|
||||
importers.insert(
|
||||
importer_id.to_string(),
|
||||
ProjectSnapshot { dependencies: Some(deps), ..ProjectSnapshot::default() },
|
||||
);
|
||||
}
|
||||
|
||||
let mut snapshots = HashMap::new();
|
||||
for peer_ver in ["1.0.0", "2.0.0"] {
|
||||
let mut tar_deps = HashMap::new();
|
||||
tar_deps.insert(pkg_name("p"), SnapshotDepRef::Plain(ver_peer(peer_ver)));
|
||||
snapshots.insert(
|
||||
dep_key("tarpkg", &format!("file:tarpkg.tgz(p@{peer_ver})")),
|
||||
SnapshotEntry { dependencies: Some(tar_deps), ..SnapshotEntry::default() },
|
||||
);
|
||||
snapshots.insert(dep_key("p", peer_ver), SnapshotEntry::default());
|
||||
}
|
||||
|
||||
let lockfile = Lockfile { importers, snapshots: Some(snapshots), ..empty_lockfile() };
|
||||
|
||||
let result =
|
||||
hoist(&lockfile, &HoistOpts::default()).expect("tarball-variant hoist should succeed");
|
||||
let root_children = result.dependencies.borrow();
|
||||
let tarpkg = &root_children.iter().find(|dep| dep.0.name == "tarpkg").unwrap().0;
|
||||
assert!(
|
||||
tarpkg.references.borrow().contains("tarpkg@file:tarpkg.tgz(p@1.0.0)"),
|
||||
"the first-seen variant is the canonical reference: {tarpkg:#?}",
|
||||
);
|
||||
for importer in ["packages%2Fa", "packages%2Fb"] {
|
||||
let importer_node = &root_children.iter().find(|dep| dep.0.name == importer).unwrap().0;
|
||||
assert!(
|
||||
!importer_node.dependencies.borrow().iter().any(|dep| dep.0.name == "tarpkg"),
|
||||
"a tarball peer variant must dedup against the root copy: {importer_node:#?}",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// A nearer ancestor holding a different version of a name blocks
|
||||
/// both hoisting and same-node dedup for that name (upstream's
|
||||
/// "filled by parent" scan): removing the edge would make this
|
||||
|
||||
@@ -134,3 +134,57 @@ function peerVariantLockfile (): LockfileObject {
|
||||
},
|
||||
} as unknown as LockfileObject
|
||||
}
|
||||
|
||||
// Peer variants of an injected directory dependency are exempt from the
|
||||
// collapse (see `getHoisterPkgId`), so the walk has to keep a location
|
||||
// per variant, where every collapsed package funnels into one.
|
||||
test('lockfileToHoistedDepGraph keeps file-dep peer variants apart', async () => {
|
||||
const dir = tempDir(false)
|
||||
const opts = hoistedOpts(dir)
|
||||
opts.storeController = {
|
||||
fetchPackage: () => ({ filesIndexFile: '' }),
|
||||
getFilesIndexFilePath: () => ({ filesIndexFile: '' }),
|
||||
} as unknown as typeof opts.storeController
|
||||
|
||||
const { graph } = await lockfileToHoistedDepGraph(fileVariantLockfile(), null, opts)
|
||||
|
||||
const compDirs = Object.keys(graph).filter((dir) => path.basename(dir) === 'comp')
|
||||
expect(compDirs).toHaveLength(2)
|
||||
const peerVersionByVariant = Object.fromEntries(compDirs.map((dir) => {
|
||||
const variant = graph[dir].depPath
|
||||
const peerDir = graph[dir].children.peer
|
||||
return [variant, graph[peerDir].depPath]
|
||||
}))
|
||||
expect(peerVersionByVariant).toStrictEqual({
|
||||
'comp@file:comp(peer@1.0.0)': 'peer@1.0.0',
|
||||
'comp@file:comp(peer@2.0.0)': 'peer@2.0.0',
|
||||
})
|
||||
})
|
||||
|
||||
function fileVariantLockfile (): LockfileObject {
|
||||
const importer = (peerVersion: string) => ({
|
||||
dependencies: {
|
||||
comp: `file:comp(peer@${peerVersion})`,
|
||||
peer: peerVersion,
|
||||
},
|
||||
specifiers: { comp: 'workspace:*', peer: peerVersion },
|
||||
})
|
||||
const compVariant = (peerVersion: string) => ({
|
||||
resolution: { directory: 'comp', type: 'directory' },
|
||||
dependencies: { peer: peerVersion },
|
||||
})
|
||||
return {
|
||||
lockfileVersion: '9.0',
|
||||
importers: {
|
||||
'.': { specifiers: {} },
|
||||
'node_modules/.bit_roots/r1': importer('1.0.0'),
|
||||
'node_modules/.bit_roots/r2': importer('2.0.0'),
|
||||
},
|
||||
packages: {
|
||||
'comp@file:comp(peer@1.0.0)': compVariant('1.0.0'),
|
||||
'comp@file:comp(peer@2.0.0)': compVariant('2.0.0'),
|
||||
'peer@1.0.0': { resolution: { integrity: 'sha512-deadbeef' } },
|
||||
'peer@2.0.0': { resolution: { integrity: 'sha512-deadbeef' } },
|
||||
},
|
||||
} as unknown as LockfileObject
|
||||
}
|
||||
@@ -40,8 +40,25 @@ export type { HoisterResult }
|
||||
* `(alias, depPath)`, so an alias exposing a package under a second
|
||||
* name gets a node, and a directory, of its own. An index over the
|
||||
* result holds the list and resolves an edge to its first entry.
|
||||
*
|
||||
* An injected directory dependency (a `directory` resolution) keeps
|
||||
* its peer suffix: every variant of it is a separate on-disk copy of
|
||||
* the local package, materialized with its own peer-resolved
|
||||
* dependency set, so collapsing the variants would rewire every
|
||||
* dependent of the losing one onto the survivor's children (Bit's
|
||||
* root components pin conflicting peers across such copies on
|
||||
* purpose). The registry collapse exists to stop peer-variant
|
||||
* explosion on large lockfiles; directory snapshots are one per
|
||||
* injected workspace package and cannot explode that way.
|
||||
*/
|
||||
export function getHoisterPkgId (depPath: string, pkgSnapshot: PackageSnapshot): string {
|
||||
// `resolution` is typed as required, but the lockfile is parsed from
|
||||
// untyped YAML — guard so a malformed snapshot degrades to the
|
||||
// collapsed identity instead of a TypeError here.
|
||||
const resolution = pkgSnapshot.resolution as PackageSnapshot['resolution'] | undefined
|
||||
if (resolution != null && 'directory' in resolution && resolution.directory != null) {
|
||||
return depPath
|
||||
}
|
||||
const { name, version } = nameVerFromPkgSnapshot(depPath, pkgSnapshot)
|
||||
return `${name}@${version}`
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user