diff --git a/src/install/lockfile/bun.lock.rs b/src/install/lockfile/bun.lock.rs index 379de751af9b..30dedb4850d5 100644 --- a/src/install/lockfile/bun.lock.rs +++ b/src/install/lockfile/bun.lock.rs @@ -1503,38 +1503,6 @@ impl PkgMap { dep: &Dependency, string_buf: &[u8], path_buf: &mut [u8], - ) -> Result<&T, ResolveError> { - self.find_resolution_impl(pkg_path, dep, string_buf, path_buf, None) - } - - /// Like `find_resolution`, but stops the upward walk one level above a - /// bundled package, mirroring `Tree::hoist_dependency`, which never - /// searches past a bundled dependency's hoist root when it re-derives - /// optional peer edges (#37346). - /// - /// Only `"bundled": true` entries bound the walk: a transitive dependency - /// of a bundled package also inherits the bundle's hoist root in - /// `Tree.rs`, but its lockfile path (`a/c`, no bundled marker) is - /// indistinguishable from an ordinary conflict-nested package whose hoist - /// root is the lockfile root. - fn find_resolution_bounded_at_bundle( - &self, - pkg_path: &[u8], - dep: &Dependency, - string_buf: &[u8], - path_buf: &mut [u8], - bundled_pkgs: &PkgPathSet, - ) -> Result<&T, ResolveError> { - self.find_resolution_impl(pkg_path, dep, string_buf, path_buf, Some(bundled_pkgs)) - } - - fn find_resolution_impl( - &self, - pkg_path: &[u8], - dep: &Dependency, - string_buf: &[u8], - path_buf: &mut [u8], - bundled_pkgs: Option<&PkgPathSet>, ) -> Result<&T, ResolveError> { let dep_name = dep.name.slice(string_buf); @@ -1546,7 +1514,6 @@ impl PkgMap { path_buf[pkg_path.len()] = b'/'; let mut offset = pkg_path.len() + 1; - let mut at_bundle_root = false; let mut valid = true; while valid { path_buf[offset..offset + dep_name.len()].copy_from_slice(dep_name); @@ -1556,14 +1523,10 @@ impl PkgMap { return Ok(entry); } - if offset == 0 || at_bundle_root { + if offset == 0 { return Err(ResolveError::Unresolvable); } - if let Some(bundled_pkgs) = bundled_pkgs { - at_bundle_root = bundled_pkgs.contains(&path_buf[0..offset - 1]); - } - let Some(slash) = strings::last_index_of_char(&path_buf[0..offset - 1], b'/') else { offset = 0; continue; @@ -2824,6 +2787,18 @@ pub(crate) fn parse_into_binary_lockfile( let dep_id: DependencyID = _dep_id; let dep = &mut dependencies[dep_id as usize]; + // Optional peers (here and in the two loops below) are left for + // `lockfile.resolve` to bind, as the fresh resolver and + // `Package::clone` leave them to the hoister. An edge bound from + // the printed paths hoists differently (it ignores the hoist + // root of a bundled dependency's subtree and places its target + // earlier), so the tree built here would differ from the one + // `clean_with_logger` rebuilds, which `Lockfile::eql` reports as + // a changed lockfile (#37346). + if dep.behavior.is_optional_peer() { + continue; + } + let peer_res_id = if is_deferred_peer(dep) { resolve_peer_dep_version_based( dep, @@ -2886,6 +2861,9 @@ pub(crate) fn parse_into_binary_lockfile( for _dep_id in deps.begin()..deps.end() { let dep_id: DependencyID = _dep_id; let dep = &mut dependencies[dep_id as usize]; + if dep.behavior.is_optional_peer() { + continue; + } let dep_name = dep.name.slice(string_buf); let workspace_node_modules = { @@ -2977,6 +2955,9 @@ pub(crate) fn parse_into_binary_lockfile( 'deps: for _dep_id in deps.begin()..deps.end() { let dep_id: DependencyID = _dep_id; let dep = &mut dependencies[dep_id as usize]; + if dep.behavior.is_optional_peer() { + continue; + } let peer_res_id = if is_deferred_peer(dep) { resolve_peer_dep_version_based( @@ -2992,21 +2973,8 @@ pub(crate) fn parse_into_binary_lockfile( let res_id = match peer_res_id { Some(id) => id, None => { - // Bounded so the loaded lockfile binds optional peers - // exactly like the hoister that re-derives them after - // `Package::clone` resets them (#37346). - let found = if dep.behavior.is_optional_peer() { - pkg_map.find_resolution_bounded_at_bundle( - pkg_path, - dep, - string_buf, - &mut path_buf[..], - &bundled_pkgs, - ) - } else { - pkg_map.find_resolution(pkg_path, dep, string_buf, &mut path_buf[..]) - }; - match found { + match pkg_map.find_resolution(pkg_path, dep, string_buf, &mut path_buf[..]) + { Ok(&id) => id, Err(ResolveError::InvalidPackageKey) => { log.add_error(Some(source), row.key_loc, b"Invalid package path"); @@ -3055,10 +3023,11 @@ pub(crate) fn parse_into_binary_lockfile( /// True for peer edges the fresh resolver defers to its second phase /// (`install_peer`) and binds by version there. Two exemptions, matching /// `enqueue_dependency_with_main_and_success_fn`: optional peers return -/// before the deferred phase and are bound to the hoisted-tree sibling by -/// `process_subtree` instead, and `*` peers express no version preference -/// and bind to whatever sibling pin existed first. Both of those are -/// exactly what the printed tree's path walk reproduces, so they keep it. +/// before the deferred phase and are only ever bound by `process_subtree` +/// (the loops in `parse_into_binary_lockfile` skip them so `Lockfile::resolve` +/// binds them here as well), and `*` peers express no version preference +/// and bind to whatever sibling pin existed first, which is exactly what the +/// printed tree's path walk reproduces, so they keep it. fn is_deferred_peer(dep: &Dependency) -> bool { dep.behavior.is_peer() && !dep.behavior.is_optional_peer() diff --git a/test/cli/install/bun-lock.test.ts b/test/cli/install/bun-lock.test.ts index f29879195094..24419976f06d 100644 --- a/test/cli/install/bun-lock.test.ts +++ b/test/cli/install/bun-lock.test.ts @@ -605,6 +605,44 @@ it("should pass frozen lockfile check when a bundled dependency has an optional expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile); }); +it("should pass frozen lockfile check when a dependency of a bundled dependency has an optional peer satisfiable from the root", async () => { + // Same as above, one level deeper: the package with the optional peer is not + // bundled itself, it is a dependency of the bundled package. It inherits the + // bundle's hoist root but its lockfile entry carries no "bundled" marker, so + // the loader must derive the binding the same way a fresh install does. + const { packageDir, packageJson } = await registry.createTestDir(); + + await write( + packageJson, + JSON.stringify({ + name: "frozen-bundled-transitive-optional-peer", + dependencies: { + // bundles `depends-on-optional-peer-deps`, which depends on + // `optional-peer-deps`, which has an optional peer on `no-deps` + "bundled-transitive-optional-peer": "1.0.0", + "no-deps": "1.0.0", + }, + }), + ); + + await runBunInstall(env, packageDir); + const lockfile = await file(join(packageDir, "bun.lock")).text(); + expect(lockfile).toContain('"bundled-transitive-optional-peer/depends-on-optional-peer-deps": '); + expect(lockfile).toContain('"bundled": true'); + // hoisted to the bundle root, no bundled marker of its own + expect(lockfile).toContain('"bundled-transitive-optional-peer/optional-peer-deps": '); + // the optional peer is not satisfiable from inside the bundle + expect(lockfile).not.toContain('"bundled-transitive-optional-peer/no-deps"'); + + await runBunInstall(env, packageDir, { frozenLockfile: true }); + + // and from a cold start with no node_modules + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + await runBunInstall(env, packageDir, { frozenLockfile: true }); + + expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile); +}); + it("should include unused resolutions in the lockfile", async () => { const { packageDir, packageJson } = await registry.createTestDir(); diff --git a/test/cli/install/registry/packages/bundled-transitive-optional-peer/bundled-transitive-optional-peer-1.0.0.tgz b/test/cli/install/registry/packages/bundled-transitive-optional-peer/bundled-transitive-optional-peer-1.0.0.tgz new file mode 100644 index 000000000000..732c8eece84a Binary files /dev/null and b/test/cli/install/registry/packages/bundled-transitive-optional-peer/bundled-transitive-optional-peer-1.0.0.tgz differ diff --git a/test/cli/install/registry/packages/bundled-transitive-optional-peer/package.json b/test/cli/install/registry/packages/bundled-transitive-optional-peer/package.json new file mode 100644 index 000000000000..1f6ac85c3b34 --- /dev/null +++ b/test/cli/install/registry/packages/bundled-transitive-optional-peer/package.json @@ -0,0 +1,44 @@ +{ + "name": "bundled-transitive-optional-peer", + "versions": { + "1.0.0": { + "name": "bundled-transitive-optional-peer", + "version": "1.0.0", + "dependencies": { + "depends-on-optional-peer-deps": "1.0.0" + }, + "bundleDependencies": [ + "depends-on-optional-peer-deps" + ], + "_id": "bundled-transitive-optional-peer@1.0.0", + "integrity": "sha512-Vp931BgUZR2t8zHh5Fttkf1qK1axqKzs1CKP0uuYCTkIjmM36hibGR6FSDeAra0FSKkHpVQJ5ApOCUewGpOWpA==", + "shasum": "2f7810e5fb282ca0622f2695a24a4d1e5ae0aab4", + "dist": { + "integrity": "sha512-Vp931BgUZR2t8zHh5Fttkf1qK1axqKzs1CKP0uuYCTkIjmM36hibGR6FSDeAra0FSKkHpVQJ5ApOCUewGpOWpA==", + "shasum": "2f7810e5fb282ca0622f2695a24a4d1e5ae0aab4", + "tarball": "http://localhost:4873/bundled-transitive-optional-peer/-/bundled-transitive-optional-peer-1.0.0.tgz" + }, + "contributors": [] + } + }, + "time": { + "modified": "2024-12-30T18:14:22.143Z", + "created": "2024-12-30T18:14:22.143Z", + "1.0.0": "2024-12-30T18:14:22.143Z" + }, + "users": {}, + "dist-tags": { + "latest": "1.0.0" + }, + "_uplinks": {}, + "_distfiles": {}, + "_attachments": { + "bundled-transitive-optional-peer-1.0.0.tgz": { + "shasum": "2f7810e5fb282ca0622f2695a24a4d1e5ae0aab4", + "version": "1.0.0" + } + }, + "_rev": "", + "_id": "bundled-transitive-optional-peer", + "readme": "" +} diff --git a/test/cli/install/registry/packages/depends-on-optional-peer-deps/depends-on-optional-peer-deps-1.0.0.tgz b/test/cli/install/registry/packages/depends-on-optional-peer-deps/depends-on-optional-peer-deps-1.0.0.tgz new file mode 100644 index 000000000000..c0543181a783 Binary files /dev/null and b/test/cli/install/registry/packages/depends-on-optional-peer-deps/depends-on-optional-peer-deps-1.0.0.tgz differ diff --git a/test/cli/install/registry/packages/depends-on-optional-peer-deps/package.json b/test/cli/install/registry/packages/depends-on-optional-peer-deps/package.json new file mode 100644 index 000000000000..58525907b673 --- /dev/null +++ b/test/cli/install/registry/packages/depends-on-optional-peer-deps/package.json @@ -0,0 +1,41 @@ +{ + "name": "depends-on-optional-peer-deps", + "versions": { + "1.0.0": { + "name": "depends-on-optional-peer-deps", + "version": "1.0.0", + "dependencies": { + "optional-peer-deps": "1.0.0" + }, + "_id": "depends-on-optional-peer-deps@1.0.0", + "integrity": "sha512-JAwdCYbJ9RphVAJ0JFW0DmlKK/7+p15QOFwA6BrGTliby76L/eyoncD26P7j5OVW9/AddxHPeo3wPhZX6l85rQ==", + "shasum": "48d7723879a9a74ba625468f9b493affe14fcd3f", + "dist": { + "integrity": "sha512-JAwdCYbJ9RphVAJ0JFW0DmlKK/7+p15QOFwA6BrGTliby76L/eyoncD26P7j5OVW9/AddxHPeo3wPhZX6l85rQ==", + "shasum": "48d7723879a9a74ba625468f9b493affe14fcd3f", + "tarball": "http://localhost:4873/depends-on-optional-peer-deps/-/depends-on-optional-peer-deps-1.0.0.tgz" + }, + "contributors": [] + } + }, + "time": { + "modified": "2024-12-30T18:14:22.143Z", + "created": "2024-12-30T18:14:22.143Z", + "1.0.0": "2024-12-30T18:14:22.143Z" + }, + "users": {}, + "dist-tags": { + "latest": "1.0.0" + }, + "_uplinks": {}, + "_distfiles": {}, + "_attachments": { + "depends-on-optional-peer-deps-1.0.0.tgz": { + "shasum": "48d7723879a9a74ba625468f9b493affe14fcd3f", + "version": "1.0.0" + } + }, + "_rev": "", + "_id": "depends-on-optional-peer-deps", + "readme": "" +} diff --git a/test/integration/next-pages/test/__snapshots__/dev-server-ssr-100.test.ts.snap b/test/integration/next-pages/test/__snapshots__/dev-server-ssr-100.test.ts.snap index 3cd4b4ce78dc..6e02f75c4986 100644 --- a/test/integration/next-pages/test/__snapshots__/dev-server-ssr-100.test.ts.snap +++ b/test/integration/next-pages/test/__snapshots__/dev-server-ssr-100.test.ts.snap @@ -26725,7 +26725,7 @@ exports[`ssr works for 100-ish requests 1`] = ` "package_id": 315, }, "jiti": { - "id": 434, + "id": 937, "package_id": 316, }, "js-tokens": { diff --git a/test/integration/next-pages/test/__snapshots__/dev-server.test.ts.snap b/test/integration/next-pages/test/__snapshots__/dev-server.test.ts.snap index 355ec90bb530..6f77a41c6fa5 100644 --- a/test/integration/next-pages/test/__snapshots__/dev-server.test.ts.snap +++ b/test/integration/next-pages/test/__snapshots__/dev-server.test.ts.snap @@ -26725,7 +26725,7 @@ exports[`hot reloading works on the client (+ tailwind hmr) 1`] = ` "package_id": 315, }, "jiti": { - "id": 434, + "id": 937, "package_id": 316, }, "js-tokens": { diff --git a/test/integration/next-pages/test/__snapshots__/next-build.test.ts.snap b/test/integration/next-pages/test/__snapshots__/next-build.test.ts.snap index a5c6c56c8b3a..77966fad2d8b 100644 --- a/test/integration/next-pages/test/__snapshots__/next-build.test.ts.snap +++ b/test/integration/next-pages/test/__snapshots__/next-build.test.ts.snap @@ -26725,7 +26725,7 @@ exports[`next build works: bun 1`] = ` "package_id": 315, }, "jiti": { - "id": 434, + "id": 937, "package_id": 316, }, "js-tokens": { @@ -54704,7 +54704,7 @@ exports[`next build works: node 1`] = ` "package_id": 315, }, "jiti": { - "id": 434, + "id": 937, "package_id": 316, }, "js-tokens": {