From 0fe89f195d0f68e12c83e26c16b329416d415d5d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 02:52:34 +0000 Subject: [PATCH 1/3] install: let the hoisted tree tell which packages a non-optional dependency is bound to, peers included The tree builder records the package that each linked dependency is placed as or deduplicated onto, for the install tree only. A peer that the hoister binds to an ancestor's other version marks that version. A peer deduplicated onto its own parent's entry of the same name does not mark: that entry decides. The hoisted install gives that bit set to RequiredPackages, which answers from it for every package the hoist saw and walks the lockfile only for a package appended later. --- src/install/hoisted_install.rs | 9 +- src/install/lockfile.rs | 49 +++++--- src/install/lockfile/Tree.rs | 103 +++++++++++---- test/cli/install/bun-install-retry.test.ts | 139 +++++++++++++++++++++ 4 files changed, 257 insertions(+), 43 deletions(-) diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index 45bc4f7c2c06..f53ac9a724db 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -76,7 +76,7 @@ pub(crate) fn install_hoisted_packages( .hoisted_dependencies .clone_from(&original_tree_dep_ids); - { + let tree_required_packages = { // `lockfile` is `Box` so the heap object // is disjoint from the `PackageManager` struct; snapshot raw `*mut // Lockfile` and `*mut Log` first so `filter` can hold `&mut Lockfile` @@ -95,9 +95,9 @@ pub(crate) fn install_hoisted_packages( install_root_dependencies, workspace_filters, packages_to_install, - )?; + )? } - } + }; // Re-derive after `filter()` so every subsequent `this` use (progress // setup through the install loop) is a fresh child of `mgr_ptr` under // Stacked Borrows — `&mut *mgr_ptr` inside the block above popped the @@ -384,7 +384,8 @@ pub(crate) fn install_hoisted_packages( summary: &mut summary, force_install, successfully_installed: Bitset::init_empty(pkg_len)?, - required_packages: tree::RequiredPackages::new( + required_packages: tree::RequiredPackages::from_tree( + tree_required_packages, workspace_filters, install_root_dependencies, packages_to_install, diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index e753842367a5..42edaa2b29b0 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -1375,10 +1375,14 @@ impl<'a> Cloner<'a> { impl Lockfile { /// Re-hoists while a pass bound an optional 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)? {} + while self + .hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)? + .late_bound_optional_peer + {} Ok(()) } + /// Returns the packages that the install tree binds a non-optional dependency to. pub(crate) fn filter( &mut self, log: &mut bun_ast::Log, @@ -1386,18 +1390,19 @@ impl Lockfile { install_root_dependencies: bool, workspace_filters: &[WorkspaceFilter], packages_to_install: Option<&[PackageID]>, - ) -> Result<(), tree::SubtreeError> { - self.hoist::<{ tree::BuilderMethod::Filter }>( - log, - Some(manager), - install_root_dependencies, - workspace_filters, - packages_to_install, - )?; - Ok(()) - } - - /// Sets `buffers.trees`/`hoisted_dependencies`; returns `Builder::late_bound_optional_peer`. + ) -> Result { + Ok(self + .hoist::<{ tree::BuilderMethod::Filter }>( + log, + Some(manager), + install_root_dependencies, + workspace_filters, + packages_to_install, + )? + .required_packages) + } + + /// Sets `buffers.trees`/`hoisted_dependencies`. pub(crate) fn hoist( &mut self, log: &mut bun_ast::Log, @@ -1407,7 +1412,7 @@ impl Lockfile { install_root_dependencies: bool, workspace_filters: &[WorkspaceFilter], packages_to_install: Option<&[PackageID]>, - ) -> Result { + ) -> Result { let slice = self.packages.slice(); // Only the install applies the barrier, so the saved tree does not depend on it. @@ -1440,6 +1445,11 @@ impl Lockfile { late_bound_optional_peer: false, list: Default::default(), sort_buf: Default::default(), + required_packages: if METHOD == tree::BuilderMethod::Filter { + DynamicBitSet::init_empty(slice.len())? + } else { + DynamicBitSet::default() + }, }; Tree::default().process_subtree(tree::ROOT_DEP_ID, tree::INVALID_ID, &mut builder)?; @@ -1459,10 +1469,19 @@ impl Lockfile { let late_bound_optional_peer = builder.late_bound_optional_peer; self.buffers.trees = cleaned.trees; self.buffers.hoisted_dependencies = cleaned.dep_ids; - Ok(late_bound_optional_peer) + Ok(HoistResult { + late_bound_optional_peer, + required_packages: cleaned.required_packages, + }) } } +pub(crate) struct HoistResult { + pub(crate) late_bound_optional_peer: bool, + /// `Builder::required_packages`. Empty unless `METHOD` is `Filter`. + pub(crate) required_packages: DynamicBitSet, +} + #[derive(Clone, Copy)] pub struct PendingResolution { pub(crate) old_resolution: PackageID, diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index 177bcedb8727..98d3c6c878ee 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -123,7 +123,11 @@ impl Tree { enum HoistDependencyResult { DependencyLoop, - Hoisted, + /// Deduplicated onto a placed dependency on this package. + Hoisted(PackageID), + /// Deduplicated onto a placed dependency of the same parent with the same name. That + /// dependency's group decides, for example an `optionalDependencies` entry over a peer. + HoistedOntoSibling, Resolve(PackageID), ResolveReplace(ResolveReplace), ResolveLater, @@ -443,6 +447,9 @@ pub struct Builder<'a, const METHOD: BuilderMethod> { pub(crate) packages_to_install: Option<&'a [PackageID]>, /// Workspace package ids that are hoisting barriers (self-contained node_modules). pub(crate) self_contained: Vec, + /// The packages that a dependency without `Behavior::OPTIONAL` is placed as or deduplicated + /// onto. A peer can be bound to another package than the one it resolves to. Only `Filter`. + pub(crate) required_packages: DynamicBitSet, } pub struct BuilderEntry { @@ -460,6 +467,7 @@ bun_collections::multi_array_columns! { pub(crate) struct CleanResult { pub trees: Vec, pub dep_ids: Vec, + pub required_packages: DynamicBitSet, } impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> { @@ -487,6 +495,18 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> { self.lockfile().buffers.string_bytes.as_slice() } + /// `dependency` is linked, bound to `pkg_id`. + fn mark_required(&mut self, dependency: &Dependency, pkg_id: PackageID) { + if METHOD == BuilderMethod::Filter + && !dependency + .behavior + .contains(crate::dependency::Behavior::OPTIONAL) + && (pkg_id as usize) < self.required_packages.bit_length() + { + self.required_packages.set(pkg_id as usize); + } + } + /// Flatten the multi-dimensional ArrayList of package IDs into a single easily serializable array pub(crate) fn clean(&mut self) -> Result { let mut total: u32 = 0; @@ -529,7 +549,11 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> { slice.deinit_owned(); - Ok(CleanResult { trees, dep_ids }) + Ok(CleanResult { + trees, + dep_ids, + required_packages: core::mem::take(&mut self.required_packages), + }) } } @@ -620,6 +644,9 @@ pub(crate) struct RequiredPackages<'a> { workspace_filters: &'a [WorkspaceFilter], install_root_dependencies: bool, packages_to_install: Option<&'a [PackageID]>, + /// `Builder::required_packages` of the hoisted install tree. It knows the package each + /// peer is bound to. A package appended after the hoist is past its end. + tree: DynamicBitSet, /// Walked on the first question that the asking dependency cannot answer. packages: Option, } @@ -629,11 +656,27 @@ impl<'a> RequiredPackages<'a> { workspace_filters: &'a [WorkspaceFilter], install_root_dependencies: bool, packages_to_install: Option<&'a [PackageID]>, + ) -> Self { + Self::from_tree( + DynamicBitSet::default(), + workspace_filters, + install_root_dependencies, + packages_to_install, + ) + } + + /// `tree` is what `Lockfile::filter` returns. + pub(crate) fn from_tree( + tree: DynamicBitSet, + workspace_filters: &'a [WorkspaceFilter], + install_root_dependencies: bool, + packages_to_install: Option<&'a [PackageID]>, ) -> Self { Self { workspace_filters, install_root_dependencies, packages_to_install, + tree, packages: None, } } @@ -652,6 +695,9 @@ impl<'a> RequiredPackages<'a> { { return true; } + if (package_id as usize) < self.tree.bit_length() { + return self.tree.is_set(package_id as usize); + } // Walk again if the install appended a package since. let packages = match &mut self.packages { Some(packages) if packages.bit_length() == lockfile.packages.len() => packages, @@ -894,20 +940,22 @@ impl Tree { if pkg_resolutions[pkg_id as usize].tag == crate::resolution::Tag::Folder { // A peer an ancestor edge already provides dedupes instead of nesting // a second copy of the folder (#40561). - if dependency.behavior.is_peer() - && matches!( - Tree::hoist_dependency::( - next_id, - hoist_root_id, - pkg_id, - dep_id, - resolution_list, - builder, - ), - HoistDependencyResult::Hoisted - ) - { - break 'hoisted HoistDependencyResult::Hoisted; + if dependency.behavior.is_peer() { + let hoisted = Tree::hoist_dependency::( + next_id, + hoist_root_id, + pkg_id, + dep_id, + resolution_list, + builder, + ); + if matches!( + hoisted, + HoistDependencyResult::Hoisted(_) + | HoistDependencyResult::HoistedOntoSibling + ) { + break 'hoisted hoisted; + } } // Folder packages never hoist, so a cycle between them would nest forever. @@ -941,7 +989,12 @@ impl Tree { }; match hoisted { - HoistDependencyResult::DependencyLoop | HoistDependencyResult::Hoisted => continue, + HoistDependencyResult::DependencyLoop + | HoistDependencyResult::HoistedOntoSibling => continue, + HoistDependencyResult::Hoisted(bound_pkg_id) => { + builder.mark_required(dependency, bound_pkg_id); + continue; + } HoistDependencyResult::Resolve(res_id) => { debug_assert!(pkg_id == invalid_package_id); @@ -972,6 +1025,7 @@ impl Tree { } HoistDependencyResult::ResolveReplace(replace) => { debug_assert!(pkg_id != invalid_package_id); + builder.mark_required(dependency, pkg_id); builder.late_bound_optional_peer = true; builder.resolutions[replace.dep_id as usize] = pkg_id; if let Some(entry) = builder @@ -1026,6 +1080,7 @@ impl Tree { entry.value_ptr.put(dep_id, ())?; } HoistDependencyResult::Placement(dest) => { + builder.mark_required(dependency, pkg_id); { // Go through ListExt // accessors sequentially so the &mut borrows do not overlap. @@ -1122,14 +1177,14 @@ impl Tree { return HoistDependencyResult::Resolve(res_id); // 1 } - if res_id == package_id { - // this dependency is the same package as the other, hoist - return HoistDependencyResult::Hoisted; // 1 - } - if input_dep_range.contains(dep_id) { // same package lists this name in another dependency group - return HoistDependencyResult::Hoisted; // 1 + return HoistDependencyResult::HoistedOntoSibling; // 1 + } + + if res_id == package_id { + // this dependency is the same package as the other, hoist + return HoistDependencyResult::Hoisted(res_id); // 1 } // now we either keep the dependency at this place in the tree, @@ -1142,7 +1197,7 @@ impl Tree { { HoistDependencyResult::Rebind(res_id) } else { - HoistDependencyResult::Hoisted + HoistDependencyResult::Hoisted(res_id) } }; diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index e15ebc04c56a..dd499402cd0f 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -751,3 +751,142 @@ describe.each(["hoisted", "isolated"])("linker=%s", linker => { }); }); }); + +// The hoisted linker places a package once, under one of the dependencies on it, +// and the failed download of that package is an error only if that dependency is +// required. The result must not depend on which dependency owns the slot. +describe.concurrent("hoisted linker: a failed download of a package that a peer needs", () => { + // The registry serves `packages`. Once `fail()` is called, that tarball answers 404. + async function registry(packages: ({ name: string; version: string } & Record)[]) { + const ctx = await createTestContext({ linker: "hoisted" }); + const registry = ctx.registry_url.slice(0, -1); + const failing = new Set(); + const tarballs = new Map(); + for (const pkg of packages) { + tarballs.set( + `/${pkg.name}-${pkg.version}.tgz`, + await new Bun.Archive({ "package/package.json": JSON.stringify(pkg) }, { compress: "gzip" }).bytes(), + ); + } + setContextHandler(ctx, request => { + const pathname = new URL(request.url).pathname.slice(`/${ctx.id}`.length); + if (tarballs.has(pathname)) { + if (failing.has(pathname)) return new Response("no", { status: 404 }); + return new Response(tarballs.get(pathname)); + } + const name = pathname.slice(1); + const versions = packages.filter(pkg => pkg.name === name); + if (versions.length === 0) return new Response("unexpected", { status: 404 }); + return Response.json({ + name, + "dist-tags": { latest: versions[0].version }, + versions: Object.fromEntries( + versions.map(pkg => [pkg.version, { ...pkg, dist: { tarball: `${registry}/${name}-${pkg.version}.tgz` } }]), + ), + }); + }); + return { + dir: ctx.package_dir, + tarball: (name: string, version: string) => `${registry}/${name}-${version}.tgz`, + fail: (name: string, version: string) => failing.add(`/${name}-${version}.tgz`), + [Symbol.dispose]: () => destroyTestContext(ctx), + }; + } + + async function install(dir: string) { + await using proc = spawn({ + cmd: [bunExe(), "install", "--no-progress", "--ignore-scripts"], + cwd: dir, + stdout: "ignore", + stderr: "pipe", + env, + }); + const [err, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + const lines = err.split(/\r?\n/); + return { + warnLines: lines.filter(l => l.startsWith("warn:")), + errorLines: lines.filter(l => l.startsWith("error:")), + exitCode, + }; + } + + // Dependencies sort by name, so the parent named `aaa` owns node_modules/baz. + it.each([ + ["the parent with the peer", "aaa", "zzz"], + ["the parent with the optional dependency", "zzz", "aaa"], + ])("is an error when %s sorts first", async (_, peerHost, optionalHost) => { + using t = await registry([ + { name: peerHost, version: "1.0.0", peerDependencies: { baz: "1.0.0" } }, + { name: optionalHost, version: "1.0.0", optionalDependencies: { baz: "1.0.0" } }, + { name: "baz", version: "1.0.0" }, + ]); + await writeFile( + join(t.dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { [peerHost]: "1.0.0", [optionalHost]: "1.0.0" } }), + ); + expect(await install(t.dir)).toEqual({ warnLines: [], errorLines: [], exitCode: 0 }); + + // `cache: false` keeps the cache in node_modules/.cache, so this also empties the cache. + await rm(join(t.dir, "node_modules"), { recursive: true, force: true }); + t.fail("baz", "1.0.0"); + expect(await install(t.dir)).toEqual({ + warnLines: [], + errorLines: [`error: GET ${t.tarball("baz", "1.0.0")} - 404`], + exitCode: 1, + }); + }); + + // A root dependency provides a peer whatever its version. The peer resolves to + // baz@1.0.0 in the lockfile, but the install binds it to the root's baz@2.0.0. + it("is an error when the peer is bound to the root's optional version of the package", async () => { + using t = await registry([ + { name: "aaa", version: "1.0.0", peerDependencies: { baz: "1.0.0" } }, + { name: "baz", version: "1.0.0" }, + { name: "baz", version: "2.0.0" }, + ]); + await writeFile( + join(t.dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { aaa: "1.0.0" }, + optionalDependencies: { baz: "2.0.0" }, + }), + ); + const incorrectPeer = 'warn: incorrect peer dependency "baz@2.0.0"'; + expect(await install(t.dir)).toEqual({ warnLines: [incorrectPeer], errorLines: [], exitCode: 0 }); + expect(await file(join(t.dir, "node_modules", "baz", "package.json")).json()).toMatchObject({ version: "2.0.0" }); + + await rm(join(t.dir, "node_modules"), { recursive: true, force: true }); + t.fail("baz", "2.0.0"); + expect(await install(t.dir)).toEqual({ + warnLines: [], + errorLines: [`error: GET ${t.tarball("baz", "2.0.0")} - 404`], + exitCode: 1, + }); + }); + + // A name in both groups of one package.json is an optional peer. The + // optionalDependencies entry owns the slot and decides. + it("is a warning when the same package.json lists the peer in optionalDependencies", async () => { + using t = await registry([{ name: "baz", version: "1.0.0" }]); + await writeFile( + join(t.dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + optionalDependencies: { baz: "1.0.0" }, + peerDependencies: { baz: "1.0.0" }, + }), + ); + expect(await install(t.dir)).toEqual({ warnLines: [], errorLines: [], exitCode: 0 }); + + await rm(join(t.dir, "node_modules"), { recursive: true, force: true }); + t.fail("baz", "1.0.0"); + expect(await install(t.dir)).toEqual({ + warnLines: [`warn: GET ${t.tarball("baz", "1.0.0")} - 404`], + errorLines: [], + exitCode: 0, + }); + }); +}); From 7bd5ebe399db08f357612738fd934b3d7c6c4e9e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 02:53:15 +0000 Subject: [PATCH 2/3] install: shorten the new comments to one line each --- src/install/lockfile/Tree.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index 98d3c6c878ee..c96ae0963108 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -125,8 +125,7 @@ enum HoistDependencyResult { DependencyLoop, /// Deduplicated onto a placed dependency on this package. Hoisted(PackageID), - /// Deduplicated onto a placed dependency of the same parent with the same name. That - /// dependency's group decides, for example an `optionalDependencies` entry over a peer. + /// Deduplicated onto the same parent's entry of the same name, whose group decides. HoistedOntoSibling, Resolve(PackageID), ResolveReplace(ResolveReplace), @@ -447,8 +446,7 @@ pub struct Builder<'a, const METHOD: BuilderMethod> { pub(crate) packages_to_install: Option<&'a [PackageID]>, /// Workspace package ids that are hoisting barriers (self-contained node_modules). pub(crate) self_contained: Vec, - /// The packages that a dependency without `Behavior::OPTIONAL` is placed as or deduplicated - /// onto. A peer can be bound to another package than the one it resolves to. Only `Filter`. + /// The packages that a dependency without `Behavior::OPTIONAL` is bound to. Only `Filter`. pub(crate) required_packages: DynamicBitSet, } @@ -644,8 +642,7 @@ pub(crate) struct RequiredPackages<'a> { workspace_filters: &'a [WorkspaceFilter], install_root_dependencies: bool, packages_to_install: Option<&'a [PackageID]>, - /// `Builder::required_packages` of the hoisted install tree. It knows the package each - /// peer is bound to. A package appended after the hoist is past its end. + /// `Builder::required_packages` of the hoisted install tree. Peers are bound there. tree: DynamicBitSet, /// Walked on the first question that the asking dependency cannot answer. packages: Option, From 7fc3fd843c1eff58c2f8dced9c3244ef41d7845f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 03:09:06 +0000 Subject: [PATCH 3/3] install: a peer whose parent lists the name in optionalDependencies is an optional peer The parent's own dependency list decides, not the dependency that owns the slot. This replaces HoistedOntoSibling, which only saw the case where the parent's optional entry owned the slot. --- src/install/lockfile/Tree.rs | 53 ++++++++++++---------- test/cli/install/bun-install-retry.test.ts | 34 ++++++++++++++ 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index c96ae0963108..83be799e7495 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -125,8 +125,6 @@ enum HoistDependencyResult { DependencyLoop, /// Deduplicated onto a placed dependency on this package. Hoisted(PackageID), - /// Deduplicated onto the same parent's entry of the same name, whose group decides. - HoistedOntoSibling, Resolve(PackageID), ResolveReplace(ResolveReplace), ResolveLater, @@ -493,16 +491,30 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> { self.lockfile().buffers.string_bytes.as_slice() } - /// `dependency` is linked, bound to `pkg_id`. - fn mark_required(&mut self, dependency: &Dependency, pkg_id: PackageID) { - if METHOD == BuilderMethod::Filter - && !dependency + /// `dependency`, one of `parent_range`, is linked, bound to `pkg_id`. + fn mark_required( + &mut self, + dependency: &Dependency, + parent_range: DependencyIDSlice, + pkg_id: PackageID, + ) { + if METHOD != BuilderMethod::Filter + || dependency .behavior .contains(crate::dependency::Behavior::OPTIONAL) - && (pkg_id as usize) < self.required_packages.bit_length() + || (pkg_id as usize) >= self.required_packages.bit_length() + { + return; + } + // A peer whose parent also lists the name in `optionalDependencies` is an optional peer. + if dependency.behavior.is_peer() + && self.dependencies[parent_range.begin() as usize..parent_range.end() as usize] + .iter() + .any(|dep| dep.name_hash == dependency.name_hash && dep.behavior.is_optional()) { - self.required_packages.set(pkg_id as usize); + return; } + self.required_packages.set(pkg_id as usize); } /// Flatten the multi-dimensional ArrayList of package IDs into a single easily serializable array @@ -946,11 +958,7 @@ impl Tree { resolution_list, builder, ); - if matches!( - hoisted, - HoistDependencyResult::Hoisted(_) - | HoistDependencyResult::HoistedOntoSibling - ) { + if matches!(hoisted, HoistDependencyResult::Hoisted(_)) { break 'hoisted hoisted; } } @@ -986,10 +994,9 @@ impl Tree { }; match hoisted { - HoistDependencyResult::DependencyLoop - | HoistDependencyResult::HoistedOntoSibling => continue, + HoistDependencyResult::DependencyLoop => continue, HoistDependencyResult::Hoisted(bound_pkg_id) => { - builder.mark_required(dependency, bound_pkg_id); + builder.mark_required(dependency, resolution_list, bound_pkg_id); continue; } @@ -1022,7 +1029,7 @@ impl Tree { } HoistDependencyResult::ResolveReplace(replace) => { debug_assert!(pkg_id != invalid_package_id); - builder.mark_required(dependency, pkg_id); + builder.mark_required(dependency, resolution_list, pkg_id); builder.late_bound_optional_peer = true; builder.resolutions[replace.dep_id as usize] = pkg_id; if let Some(entry) = builder @@ -1077,7 +1084,7 @@ impl Tree { entry.value_ptr.put(dep_id, ())?; } HoistDependencyResult::Placement(dest) => { - builder.mark_required(dependency, pkg_id); + builder.mark_required(dependency, resolution_list, pkg_id); { // Go through ListExt // accessors sequentially so the &mut borrows do not overlap. @@ -1174,16 +1181,16 @@ impl Tree { return HoistDependencyResult::Resolve(res_id); // 1 } - if input_dep_range.contains(dep_id) { - // same package lists this name in another dependency group - return HoistDependencyResult::HoistedOntoSibling; // 1 - } - if res_id == package_id { // this dependency is the same package as the other, hoist return HoistDependencyResult::Hoisted(res_id); // 1 } + if input_dep_range.contains(dep_id) { + // same package lists this name in another dependency group + return HoistDependencyResult::Hoisted(res_id); // 1 + } + // now we either keep the dependency at this place in the tree, // or hoist if peer version allows it diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index dd499402cd0f..93426831a9d3 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -889,4 +889,38 @@ describe.concurrent("hoisted linker: a failed download of a package that a peer exitCode: 0, }); }); + + // The root's optional entry owns the slot, so both of the workspace's entries + // are deduplicated onto it. The workspace's own optional entry still decides. + it("is a warning when a workspace lists the peer in optionalDependencies and the root owns the slot", async () => { + using t = await registry([{ name: "baz", version: "1.0.0" }]); + await writeFile( + join(t.dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + workspaces: ["packages/*"], + optionalDependencies: { baz: "1.0.0" }, + }), + ); + await mkdir(join(t.dir, "packages", "w"), { recursive: true }); + await writeFile( + join(t.dir, "packages", "w", "package.json"), + JSON.stringify({ + name: "w", + version: "0.0.1", + optionalDependencies: { baz: "1.0.0" }, + peerDependencies: { baz: "1.0.0" }, + }), + ); + expect(await install(t.dir)).toEqual({ warnLines: [], errorLines: [], exitCode: 0 }); + + await rm(join(t.dir, "node_modules"), { recursive: true, force: true }); + t.fail("baz", "1.0.0"); + expect(await install(t.dir)).toEqual({ + warnLines: [`warn: GET ${t.tarball("baz", "1.0.0")} - 404`], + errorLines: [], + exitCode: 0, + }); + }); });