From 9605fb676d23dd149554ae7761bf7d13e3cac5f8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 16 Aug 2026 09:02:35 +0000 Subject: [PATCH] install: bind unbound required peers while hoisting, like optional peers The hoister already walks the tree for an unresolved optional peer and binds it to the copy placed next to or above its dependent. Required peers that arrive unbound (a pnpm-lock.yaml or yarn.lock migration has nothing recorded for a peer the other tool left unmet) were skipped instead, so the install that migrated keyed the dependent's isolated store entry without the peer, while every later install, which loads bun.lock and binds the edge by the same tree walk, keyed it with the peer and relinked. Walk for every unbound peer, so the migrating install ends up with the bindings a reload produces. --- src/install/lockfile.rs | 12 +- src/install/lockfile/Tree.rs | 51 +++-- .../install/migration/pnpm-lock-v9.test.ts | 199 ++++++++++++++++++ .../migration/yarn-lock-migration.test.ts | 101 ++++++++- 4 files changed, 328 insertions(+), 35 deletions(-) diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index da5691de7ee9..6f04126ac231 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -1269,7 +1269,7 @@ impl<'a> Cloner<'a> { // ──────────────────────────────────────────────────────────────────────────── impl Lockfile { - /// Re-hoists while a pass bound an optional peer late; a reload has that binding up front. + /// Re-hoists while a pass bound a peer late; a reload has that binding up front. pub(crate) fn resolve(&mut self, log: &mut bun_ast::Log) -> Result<(), tree::SubtreeError> { while self.hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)? {} Ok(()) @@ -1293,7 +1293,7 @@ impl Lockfile { Ok(()) } - /// Sets `buffers.trees`/`hoisted_dependencies`; returns `Builder::late_bound_optional_peer`. + /// Sets `buffers.trees`/`hoisted_dependencies`; returns `Builder::late_bound_peer`. pub(crate) fn hoist( &mut self, log: &mut bun_ast::Log, @@ -1324,8 +1324,8 @@ impl Lockfile { install_root_dependencies, workspace_filters, packages_to_install, - pending_optional_peers: Default::default(), - late_bound_optional_peer: false, + pending_peers: Default::default(), + late_bound_peer: false, list: Default::default(), sort_buf: Default::default(), }; @@ -1344,10 +1344,10 @@ impl Lockfile { } let cleaned = builder.clean()?; - let late_bound_optional_peer = builder.late_bound_optional_peer; + let late_bound_peer = builder.late_bound_peer; self.buffers.trees = cleaned.trees; self.buffers.hoisted_dependencies = cleaned.dep_ids; - Ok(late_bound_optional_peer) + Ok(late_bound_peer) } } diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index 7143e9260626..de54922efa7f 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -436,14 +436,13 @@ pub struct Builder<'a, const METHOD: BuilderMethod> { /// overlap; reads go through [`Builder::lockfile()`] which never touches /// `buffers.resolutions`. pub lockfile: bun_ptr::ParentRef, - // Unresolved optional peers that might resolve later. if they do we will want to assign + // Unresolved peers that might resolve later. if they do we will want to assign // builder.resolutions[peer.dep_id] to the resolved pkg_id. A dependency ID set is used because there // can be multiple instances of the same package in the tree, so the same unresolved dependency ID // could be visited multiple times before it's resolved. - pub(crate) pending_optional_peers: - ArrayHashMap>, - /// An optional peer got bound after its dependent was placed; see `Lockfile::resolve`. - pub(crate) late_bound_optional_peer: bool, + pub(crate) pending_peers: ArrayHashMap>, + /// A peer got bound after its dependent was placed; see `Lockfile::resolve`. + pub(crate) late_bound_peer: bool, pub(crate) manager: Option<&'a PackageManager>, pub(crate) sort_buf: Vec, pub(crate) workspace_filters: &'a [WorkspaceFilter], @@ -518,7 +517,7 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> { for &dep_id in child.iter() { let pkg_id = self.resolutions[dep_id as usize]; if pkg_id == invalid_package_id { - // optional peers that never resolved + // peers that never resolved continue; } @@ -530,7 +529,7 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> { tree.dependencies.len = len; } - // queue / sort_buf / pending_optional_peers freed by Drop; explicit deinit removed. + // queue / sort_buf / pending_peers freed by Drop; explicit deinit removed. // The sole caller (`Lockfile::hoist`) drops the Builder immediately after clean(). slice.deinit_owned(); @@ -768,7 +767,11 @@ impl Tree { } if pkg_id == invalid_package_id { - if dependency.behavior.is_optional_peer() { + // An unbound peer (optional, or one a migration had nothing recorded for) + // binds to the copy next to or above its dependent, which is where loading + // the saved bun.lock binds it (`PkgMap::find_resolution`); the isolated + // store keys the dependent by that binding. + if dependency.behavior.is_peer() { break 'hoisted Tree::hoist_dependency::( next_id, hoist_root_id, @@ -821,14 +824,10 @@ impl Tree { debug_assert!(pkg_id == invalid_package_id); debug_assert!(res_id != invalid_package_id); builder.resolutions[dep_id as usize] = res_id; - debug_assert!( - !builder - .pending_optional_peers - .contains_key(&dependency.name_hash) - ); + debug_assert!(!builder.pending_peers.contains_key(&dependency.name_hash)); if let Some(entry) = builder - .pending_optional_peers + .pending_peers .fetch_swap_remove(&dependency.name_hash) { let peers = entry.1; @@ -846,10 +845,10 @@ impl Tree { } HoistDependencyResult::ResolveReplace(replace) => { debug_assert!(pkg_id != invalid_package_id); - builder.late_bound_optional_peer = true; + builder.late_bound_peer = true; builder.resolutions[replace.dep_id as usize] = pkg_id; if let Some(entry) = builder - .pending_optional_peers + .pending_peers .fetch_swap_remove(&dependency.name_hash) { let peers = entry.1; @@ -887,12 +886,10 @@ impl Tree { builder.resolutions[dep_id as usize] = res_id; } HoistDependencyResult::ResolveLater => { - // `dep_id` is an unresolved optional peer. while hoisting it deduplicated - // with another unresolved optional peer. save it so we remember resolve it + // `dep_id` is an unresolved peer. while hoisting it deduplicated + // with another unresolved peer. save it so we remember resolve it // later if it's possible to resolve it. - let entry = builder - .pending_optional_peers - .get_or_put(dependency.name_hash)?; + let entry = builder.pending_peers.get_or_put(dependency.name_hash)?; if !entry.found_existing { *entry.value_ptr = ArrayHashMap::default(); } @@ -974,15 +971,15 @@ impl Tree { let res_id = builder.resolutions[dep_id as usize]; if res_id == invalid_package_id && package_id == invalid_package_id { - debug_assert!(dep.behavior.is_optional_peer()); - debug_assert!(dependency.behavior.is_optional_peer()); - // both optional peers will need to be resolved if they can resolve later. + debug_assert!(dep.behavior.is_peer()); + debug_assert!(dependency.behavior.is_peer()); + // both peers will need to be resolved if they can resolve later. // remember input package_id and dependency for later return HoistDependencyResult::ResolveLater; } if res_id == invalid_package_id { - debug_assert!(dep.behavior.is_optional_peer()); + debug_assert!(dep.behavior.is_peer()); return HoistDependencyResult::ResolveReplace(ResolveReplace { id: this.id, dep_id, @@ -990,9 +987,9 @@ impl Tree { } if package_id == invalid_package_id { - debug_assert!(dependency.behavior.is_optional_peer()); + debug_assert!(dependency.behavior.is_peer()); debug_assert!(res_id != invalid_package_id); - // resolve optional peer to `builder.resolutions[dep_id]` + // resolve peer to `builder.resolutions[dep_id]` return HoistDependencyResult::Resolve(res_id); // 1 } diff --git a/test/cli/install/migration/pnpm-lock-v9.test.ts b/test/cli/install/migration/pnpm-lock-v9.test.ts index b943197ac326..100f0c670db8 100644 --- a/test/cli/install/migration/pnpm-lock-v9.test.ts +++ b/test/cli/install/migration/pnpm-lock-v9.test.ts @@ -1663,6 +1663,205 @@ ${variants}`; ).toBeTrue(); }); + describe("a peer pnpm left unmet is bound by the install that migrates", () => { + // pnpm records a met peer as a suffix on the dependent's snapshot key; an unmet one (autoInstallPeers + // off) has no suffix, so the migration has nothing to bind peer-deps-too's `no-deps` edge to. Loading + // the migrated bun.lock binds it to the no-deps hoisted to the root, and the isolated store keys + // peer-deps-too by that binding, so the migrating install has to bind it the same way or the next + // install re-keys the entry (bun.lock itself is identical either way). + const unmetPeerProject = (importer: string, packageJson: Record) => ({ + "package.json": JSON.stringify({ name: "unmet-peer", ...packageJson }), + "pnpm-lock.yaml": `lockfileVersion: '9.0' + +settings: + autoInstallPeers: false + +importers: + + .: +${importer} + +packages: + + no-deps@1.0.1: + resolution: {integrity: ${NO_DEPS_1_0_1_INTEGRITY}} + + one-dep@1.0.0: + resolution: {integrity: ${ONE_DEP_1_0_0_INTEGRITY}} + + peer-deps-too@1.0.0: + resolution: {integrity: ${PEER_DEPS_TOO_1_0_0_INTEGRITY}} + peerDependencies: + no-deps: '*' + +snapshots: + + no-deps@1.0.1: {} + + one-dep@1.0.0: + dependencies: + no-deps: 1.0.1 + + peer-deps-too@1.0.0: {} +`, + }); + + const storeEntries = (dir: string) => + readdirSync(join(dir, "node_modules", ".bun")) + .filter(name => name !== "node_modules") + .sort(); + + // Hoisting places a package's dependencies breadth-first in dependency-group order. With both in + // `dependencies`, one-dep's no-deps reaches the root before peer-deps-too's peer is looked at; as a + // devDependency peer-deps-too is processed first, and the peer is bound when no-deps arrives later. + test.concurrent.each([ + { + group: "dependencies", + packageJson: { dependencies: { "one-dep": "1.0.0", "peer-deps-too": "1.0.0" } }, + importer: ` dependencies: + one-dep: + specifier: 1.0.0 + version: 1.0.0 + peer-deps-too: + specifier: 1.0.0 + version: 1.0.0`, + }, + { + group: "devDependencies", + packageJson: { dependencies: { "one-dep": "1.0.0" }, devDependencies: { "peer-deps-too": "1.0.0" } }, + importer: ` dependencies: + one-dep: + specifier: 1.0.0 + version: 1.0.0 + devDependencies: + peer-deps-too: + specifier: 1.0.0 + version: 1.0.0`, + }, + ])( + "peer-deps-too in $group keys its isolated store entry like a reinstall and like a fresh install", + async ({ packageJson, importer }) => { + const files = unmetPeerProject(importer, packageJson); + const { packageDir } = await verdaccio.createTestDir({ bunfigOpts: { linker: "isolated" }, files }); + + const install = await run(packageDir, "install"); + + expect(install.stderr).toContain("migrated lockfile from pnpm-lock.yaml"); + expect(install.exitCode).toBe(0); + const migrated = await bunLockOf(packageDir); + expect(migrated).toContain(`{ "peerDependencies": { "no-deps": "*" } }`); + const store = storeEntries(packageDir); + expect(store).toEqual([ + "no-deps@1.0.1", + "one-dep@1.0.0", + expect.stringMatching(/^peer-deps-too@1\.0\.0\+[0-9a-f]{16}$/), + ]); + + const reinstall = await run(packageDir, "install"); + + expect(reinstall.stdout).toContain("(no changes)"); + expect(reinstall.exitCode).toBe(0); + expect(await bunLockOf(packageDir)).toBe(migrated); + expect(storeEntries(packageDir)).toEqual(store); + + const { packageDir: fresh } = await verdaccio.createTestDir({ + bunfigOpts: { linker: "isolated" }, + files: { "package.json": files["package.json"] }, + }); + + const freshInstall = await run(fresh, "install"); + + expect(freshInstall.stderr).toContain("Saved lockfile"); + expect(freshInstall.exitCode).toBe(0); + expect(storeEntries(fresh)).toEqual(store); + }, + ); + + // The root's and a workspace's own peers come from package.json and are unbound the same way; the + // isolated linker links a bound peer into the importer's node_modules. + test.concurrent("the root's and a workspace's own peers are linked by the migrating install", async () => { + const manifests = { + "package.json": JSON.stringify({ + name: "importer-peers", + workspaces: ["apps/*"], + dependencies: { "one-dep": "1.0.0" }, + peerDependencies: { "no-deps": "*" }, + }), + "apps/a/package.json": JSON.stringify({ name: "a", peerDependencies: { "no-deps": "*" } }), + }; + const { packageDir } = await verdaccio.createTestDir({ + bunfigOpts: { linker: "isolated" }, + files: { + ...manifests, + "pnpm-lock.yaml": `lockfileVersion: '9.0' + +settings: + autoInstallPeers: false + +importers: + + .: + dependencies: + one-dep: + specifier: 1.0.0 + version: 1.0.0 + + apps/a: {} + +packages: + + no-deps@1.0.1: + resolution: {integrity: ${NO_DEPS_1_0_1_INTEGRITY}} + + one-dep@1.0.0: + resolution: {integrity: ${ONE_DEP_1_0_0_INTEGRITY}} + +snapshots: + + no-deps@1.0.1: {} + + one-dep@1.0.0: + dependencies: + no-deps: 1.0.1 +`, + }, + }); + const linked = (dir: string) => ({ + root: readdirSync(join(dir, "node_modules")) + .filter(name => name !== ".bun") + .sort(), + a: existsSync(join(dir, "apps/a/node_modules")) ? readdirSync(join(dir, "apps/a/node_modules")).sort() : [], + }); + + const install = await run(packageDir, "install"); + + expect(install.stderr).toContain('skipped peer "no-deps" of the root package'); + expect(install.stderr).toContain('skipped peer "no-deps" of workspace "apps/a"'); + expect(install.stderr).toContain("migrated lockfile from pnpm-lock.yaml"); + expect(install.exitCode).toBe(0); + const migrated = await bunLockOf(packageDir); + expect(linked(packageDir)).toEqual({ root: ["no-deps", "one-dep"], a: ["no-deps"] }); + + const reinstall = await run(packageDir, "install"); + + expect(reinstall.stdout).toContain("(no changes)"); + expect(reinstall.exitCode).toBe(0); + expect(await bunLockOf(packageDir)).toBe(migrated); + expect(linked(packageDir)).toEqual({ root: ["no-deps", "one-dep"], a: ["no-deps"] }); + + const { packageDir: fresh } = await verdaccio.createTestDir({ + bunfigOpts: { linker: "isolated" }, + files: manifests, + }); + + const freshInstall = await run(fresh, "install"); + + expect(freshInstall.stderr).toContain("Saved lockfile"); + expect(freshInstall.exitCode).toBe(0); + expect(linked(fresh)).toEqual({ root: ["no-deps", "one-dep"], a: ["no-deps"] }); + }); + }); + const linkedPeerFiles = { "package.json": JSON.stringify({ name: "v9-linked-peer", diff --git a/test/cli/install/migration/yarn-lock-migration.test.ts b/test/cli/install/migration/yarn-lock-migration.test.ts index de69143e155b..554edb596100 100644 --- a/test/cli/install/migration/yarn-lock-migration.test.ts +++ b/test/cli/install/migration/yarn-lock-migration.test.ts @@ -1,6 +1,6 @@ -import { describe, expect, test } from "bun:test"; +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; import fs from "fs"; -import { bunEnv, bunExe, tempDir } from "harness"; +import { bunEnv, bunExe, tempDir, VerdaccioRegistry } from "harness"; import { join } from "path"; describe("yarn.lock migration basic", () => { @@ -1640,3 +1640,100 @@ fsevents@^2.3.2: expect(bunLockContent).toContain("@esbuild/darwin-arm64"); }); }); + +describe("installing from a migrated yarn.lock", () => { + const verdaccio = new VerdaccioRegistry(); + const REGISTRY_PACKAGES = join(import.meta.dir, "..", "registry", "packages"); + + beforeAll(async () => { + await verdaccio.start(); + }); + + afterAll(() => { + verdaccio.stop(); + }); + + // Keyed by the exact spec that requested it, as yarn writes pinned dependencies. + function yarnEntry(name: string, version: string, body = "") { + const { shasum, integrity } = JSON.parse(fs.readFileSync(join(REGISTRY_PACKAGES, name, "package.json"), "utf8")) + .versions[version].dist; + return `${name}@${version}: + version "${version}" + resolved "${verdaccio.registryUrl()}${name}/-/${name}-${version}.tgz#${shasum}" + integrity ${integrity} +${body}`; + } + + async function install(cwd: string) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "install"], + cwd, + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + } + + const storeEntries = (dir: string) => + fs + .readdirSync(join(dir, "node_modules", ".bun")) + .filter(name => name !== "node_modules") + .sort(); + + test("a peer yarn.lock has no entry for is bound by the install that migrates", async () => { + // yarn.lock keys entries by the specs that requested them; nothing requested `no-deps@*`, so the + // migration cannot bind peer-deps-too's peer from the file. Loading the migrated bun.lock binds it to + // the no-deps at the root and the isolated store keys peer-deps-too by that binding, so the migrating + // install has to bind it the same way or the next install re-keys the entry. + const packageJson = JSON.stringify({ + name: "unmet-peer", + dependencies: { "one-dep": "1.0.0", "peer-deps-too": "1.0.0" }, + }); + const { packageDir } = await verdaccio.createTestDir({ + bunfigOpts: { linker: "isolated" }, + files: { + "package.json": packageJson, + "yarn.lock": `# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. +# yarn lockfile v1 + + +${yarnEntry("no-deps", "1.0.1")} +${yarnEntry("one-dep", "1.0.0", ` dependencies:\n no-deps "1.0.1"\n`)} +${yarnEntry("peer-deps-too", "1.0.0", ` peerDependencies:\n no-deps "*"\n`)}`, + }, + }); + + const migrating = await install(packageDir); + + expect(migrating.stderr).toContain("migrated lockfile from yarn.lock"); + expect(migrating.exitCode).toBe(0); + const migrated = fs.readFileSync(join(packageDir, "bun.lock"), "utf8"); + expect(migrated).toContain(`"peerDependencies": { "no-deps": "*" }`); + const store = storeEntries(packageDir); + expect(store).toEqual([ + "no-deps@1.0.1", + "one-dep@1.0.0", + expect.stringMatching(/^peer-deps-too@1\.0\.0\+[0-9a-f]{16}$/), + ]); + + const reinstall = await install(packageDir); + + expect(reinstall.stdout).toContain("(no changes)"); + expect(reinstall.exitCode).toBe(0); + expect(fs.readFileSync(join(packageDir, "bun.lock"), "utf8")).toBe(migrated); + expect(storeEntries(packageDir)).toEqual(store); + + const { packageDir: fresh } = await verdaccio.createTestDir({ + bunfigOpts: { linker: "isolated" }, + files: { "package.json": packageJson }, + }); + + const freshInstall = await install(fresh); + + expect(freshInstall.stderr).toContain("Saved lockfile"); + expect(freshInstall.exitCode).toBe(0); + expect(storeEntries(fresh)).toEqual(store); + }); +});