Skip to content

install: let the hoister bind optional peers when loading bun.lock #37925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

install: let the hoister bind optional peers when loading bun.lock #37925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 26 additions & 57 deletions src/install/lockfile/bun.lock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1503,38 +1503,6 @@ impl<T> PkgMap<T> {
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);

Expand All @@ -1546,7 +1514,6 @@ impl<T> PkgMap<T> {
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);
Expand All @@ -1556,14 +1523,10 @@ impl<T> PkgMap<T> {
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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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(
Expand All @@ -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");
Expand Down Expand Up @@ -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()
Expand Down
38 changes: 38 additions & 0 deletions test/cli/install/bun-lock.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
Binary file added ...stry/packages/bundled-transitive-optional-peer/bundled-transitive-optional-peer-1.0.0.tgz
View file
Open in desktop
Binary file not shown.
44 changes: 44 additions & 0 deletions test/cli/install/registry/packages/bundled-transitive-optional-peer/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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": ""
}
Binary file added ...l/registry/packages/depends-on-optional-peer-deps/depends-on-optional-peer-deps-1.0.0.tgz
View file
Open in desktop
Binary file not shown.
41 changes: 41 additions & 0 deletions test/cli/install/registry/packages/depends-on-optional-peer-deps/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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": ""
}
2 changes: 1 addition & 1 deletion test/integration/next-pages/test/__snapshots__/dev-server-ssr-100.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
2 changes: 1 addition & 1 deletion test/integration/next-pages/test/__snapshots__/dev-server.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
4 changes: 2 additions & 2 deletions test/integration/next-pages/test/__snapshots__/next-build.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26725,7 +26725,7 @@ exports[`next build works: bun 1`] = `
"package_id": 315,
},
"jiti": {
"id": 434,
"id": 937,
"package_id": 316,
},
"js-tokens": {
Expand Down Expand Up @@ -54704,7 +54704,7 @@ exports[`next build works: node 1`] = `
"package_id": 315,
},
"jiti": {
"id": 434,
"id": 937,
"package_id": 316,
},
"js-tokens": {
Expand Down