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); + }); +});