Skip to content

install: don't mark a placed peer optional when saving a migrated bun.lock #37289

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

Closed
robobun wants to merge 5 commits into from
Closed

install: don't mark a placed peer optional when saving a migrated bun.lock #37289

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
72795f0
install: don't mark a placed peer optional when saving a migrated bun…
robobun Aug 9, 2026
f21d09d
[autofix.ci] apply automated fixes
autofix-ci[bot] Aug 9, 2026
b244db3
shorten comments flagged by review
robobun Aug 9, 2026
26ccf90
compress comment to two lines
robobun Aug 9, 2026
60cb27f
reduce comment to one line
robobun Aug 9, 2026
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
55 changes: 29 additions & 26 deletions src/install/lockfile/bun.lock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,7 @@ impl Stringifier {

let mut tree_deps_sort_buf: Vec<DependencyID> = Vec::new();
let mut pkg_deps_sort_buf: Vec<DependencyID> = Vec::new();
let mut entry_path_buf: Vec<u8> = Vec::new();

Self::write_indent(writer, *indent)?;
writer.write_all(b"\"packages\": {")?;
Expand Down Expand Up @@ -707,29 +708,27 @@ impl Stringifier {
Self::write_indent(writer, *indent)?;
}

writer.write_byte(b'"')?;
// relative_path is empty string for root resolutions
write!(
writer,
"{}",
bun_core::fmt::format_json_string_utf8(
relative_path,
bun_core::fmt::JSONFormatterUTF8Options { quote: false }
),
)?;
let dep = &deps_buf[dep_id as usize];
let dep_name = dep.name.slice(buf);

// The entry key is also the tree path this package's own
// dependencies resolve from. `find_resolution` must walk up
// from it (not from the parent node's `relative_path`), the
// same way the parser binds this entry's dependencies.
// relative_path is empty string for root resolutions.
Comment thread
robobun marked this conversation as resolved.
Outdated
entry_path_buf.clear();
entry_path_buf.extend_from_slice(relative_path);
if *depth != 0 {
writer.write_byte(b'/')?;
entry_path_buf.push(b'/');
}

let dep = &deps_buf[dep_id as usize];
let dep_name = dep.name.slice(buf);
entry_path_buf.extend_from_slice(dep_name);
let entry_path: &[u8] = entry_path_buf.as_slice();

write!(
writer,
"{}\": ",
"\"{}\": ",
bun_core::fmt::format_json_string_utf8(
dep_name,
entry_path,
bun_core::fmt::JSONFormatterUTF8Options { quote: false }
),
)?;
Expand Down Expand Up @@ -826,7 +825,7 @@ impl Stringifier {
&mut optional_peers_buf,
extern_strings,
&pkg_map,
relative_path,
entry_path,
&mut path_buf,
)?;

Expand All @@ -851,7 +850,7 @@ impl Stringifier {
&mut optional_peers_buf,
extern_strings,
&pkg_map,
relative_path,
entry_path,
&mut path_buf,
)?;

Expand Down Expand Up @@ -881,7 +880,7 @@ impl Stringifier {
&mut optional_peers_buf,
extern_strings,
&pkg_map,
relative_path,
entry_path,
&mut path_buf,
)?;

Expand Down Expand Up @@ -910,7 +909,7 @@ impl Stringifier {
&mut optional_peers_buf,
extern_strings,
&pkg_map,
relative_path,
entry_path,
&mut path_buf,
)?;

Expand Down Expand Up @@ -957,7 +956,7 @@ impl Stringifier {
&mut optional_peers_buf,
extern_strings,
&pkg_map,
relative_path,
entry_path,
&mut path_buf,
)?;

Expand Down Expand Up @@ -1005,7 +1004,7 @@ impl Stringifier {
&mut optional_peers_buf,
extern_strings,
&pkg_map,
relative_path,
entry_path,
&mut path_buf,
)?;

Expand Down Expand Up @@ -1054,7 +1053,9 @@ impl Stringifier {
optional_peers_buf: &mut Vec<String>,
extern_strings: &[ExternalString],
pkg_map: &PkgMap<()>,
relative_path: &[u8],
// The package's own tree path (its `packages` key), which its
// dependencies resolve relative to; see `PkgMap::find_resolution`.
Comment thread
robobun marked this conversation as resolved.
Outdated
pkg_path: &[u8],
path_buf: &mut [u8],
) -> Result<(), WriteError> {
// `optional_peers_buf` is cleared at the fn tail.
Expand Down Expand Up @@ -1109,7 +1110,7 @@ impl Stringifier {
&& pkg_map.map.len() > 0
{
if pkg_map
.find_resolution(relative_path, dep, buf, path_buf)
.find_resolution(pkg_path, dep, buf, path_buf)
.is_err()
{
optional_peers_buf.push(dep.name);
Expand Down Expand Up @@ -1230,7 +1231,9 @@ impl Stringifier {
workspace_versions: &VersionHashMap,
optional_peers_buf: &mut Vec<String>,
pkg_map: &PkgMap<()>,
relative_path: &[u8],
// The workspace package's own tree path ("" for the root package, the
// workspace name otherwise); its dependencies resolve relative to it.
Comment thread
robobun marked this conversation as resolved.
Outdated
pkg_path: &[u8],
path_buf: &mut [u8],
) -> Result<(), WriteError> {
// `optional_peers_buf` is cleared at the fn tail.
Expand Down Expand Up @@ -1351,7 +1354,7 @@ impl Stringifier {
&& !dep.behavior.contains(Behavior::OPTIONAL)
&& pkg_map.map.len() > 0
{
if let Err(err) = pkg_map.find_resolution(relative_path, dep, buf, path_buf) {
if let Err(err) = pkg_map.find_resolution(pkg_path, dep, buf, path_buf) {
if err == ResolveError::Unresolvable {
optional_peers_buf.push(dep.name);
}
Expand Down
70 changes: 70 additions & 0 deletions test/cli/install/migration/migrate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,76 @@ test.concurrent.each([
},
);

test.concurrent("migrated bun.lock with a nested-placed peer of a file: dependency round-trips --frozen-lockfile", async () => {
// gamma (a file: dependency of the root) has a regular peer dependency on
// delta, and delta's only placement is nested (workspace ws-a's file:
// dependency), never hoisted to the root. The migration writer records the
// placement as "gamma/delta" and must not also list delta in gamma's
// "optionalPeers": reloading would then treat the peer as optional, re-derive
// its resolution slot, drop the entry, and fail --frozen-lockfile on an
// unchanged tree.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
await using testDir = tempDir("migrate-nested-peer-fixed-point", {
"package.json": JSON.stringify({
name: "sandbox",
version: "1.0.0",
workspaces: ["packages/ws-a"],
dependencies: { gamma: "file:vendor/gamma" },
}),
"vendor/gamma/package.json": JSON.stringify({
name: "gamma",
version: "1.0.0",
peerDependencies: { delta: "*" },
}),
"vendor/delta/package.json": JSON.stringify({ name: "delta", version: "2.0.0" }),
"packages/ws-a/package.json": JSON.stringify({
name: "ws-a",
version: "0.1.0",
dependencies: { delta: "file:../../vendor/delta" },
}),
// What `npm install --package-lock-only` produces for this tree.
"package-lock.json": JSON.stringify({
name: "sandbox",
version: "1.0.0",
lockfileVersion: 3,
requires: true,
packages: {
"": {
name: "sandbox",
version: "1.0.0",
dependencies: { gamma: "file:vendor/gamma" },
workspaces: ["packages/ws-a"],
},
"node_modules/gamma": { resolved: "vendor/gamma", link: true },
"vendor/gamma": { name: "gamma", version: "1.0.0", peerDependencies: { delta: "*" } },
"node_modules/delta": { resolved: "vendor/delta", link: true },
"vendor/delta": { name: "delta", version: "2.0.0" },
"node_modules/ws-a": { resolved: "packages/ws-a", link: true },
"packages/ws-a": { name: "ws-a", version: "0.1.0", dependencies: { delta: "file:../../vendor/delta" } },
},
}),
});

const first = await install(testDir);
expect(first.stderr).toContain("migrated lockfile from package-lock.json");
expect(first.exitCode).toBe(0);

const lock = await Bun.file(join(testDir, "bun.lock")).text();
// The peer's resolution is recorded right below in the same lockfile, so the
// peer is not optional.
expect(lock).toContain('"gamma/delta"');
expect(lock).not.toContain("optionalPeers");

// An unchanged tree satisfies --frozen-lockfile...
const frozen = await install(testDir, "--frozen-lockfile");
expect(frozen.stderr).not.toContain("lockfile had changes");
expect(frozen.exitCode).toBe(0);

// ...and a plain re-install does not rewrite the lockfile.
const second = await install(testDir);
expect(second.exitCode).toBe(0);
expect(await Bun.file(join(testDir, "bun.lock")).text()).toBe(lock);
});

test.concurrent("package-lock.json migration does not platform-skip a regular file: tarball dependency", async () => {
// Same divergence as the folder variant, for a `LocalTarball` resolution. npm records
// the packed package's `os`/`cpu` arrays in its lockfile entry, and a fresh resolve of
Expand Down
Loading