Repository navigation
fix(install): fix optional peer resolving - #24272
Conversation
|
Updated 10:36 PM PT - Nov 1st, 2025
❌ @dylan-conway, your commit 4fb650a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 24272That installs a local version of the PR into your bun-24272 --bun |
WalkthroughThe PR refactors optional peer dependency resolution in the isolated linker by introducing explicit resolution pathways (resolve, resolve_replace, resolve_later), invalid package ID handling, and a new pending_optional_peers tracking mechanism. Changes span installer logic, lockfile tree building, and comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Isolated Installer
participant Resolver as Peer Resolver
participant Builder as Tree Builder
participant Hoister as Hoisting Logic
Installer->>Resolver: Iterate peer dependencies
rect rgb(200, 220, 240)
Note over Resolver: Peer Resolution Phase
Resolver->>Resolver: Resolve best_version
alt Invalid Resolution
Resolver->>Installer: Mark as invalid_package_id
Installer->>Installer: Break, mark auto_installed=true
else Valid Resolution
Resolver->>Builder: Update resolutions[]
end
end
alt Package ID is invalid_package_id
Installer->>Installer: Skip and continue next peer
else Package ID is valid
Installer->>Builder: Check pending_optional_peers
Builder->>Hoister: Propagate resolution
rect rgb(240, 220, 200)
Note over Hoister: Hoisting Phase
Hoister->>Hoister: Apply placement logic
Hoister->>Hoister: Guard against invalid IDs
Hoister->>Builder: Update hoist root/queue placements
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
[pre_merge_check_pass] The code changes directly address the objectives from all three linked issues. Issue #23725 requires preventing duplicate package installations with different content hashes; the PR modifies peer resolution logic to allow optional peers to resolve, preventing duplicate copies of packages like elysia. Issue #23895 addresses isolated linker breaking type emissions; the same resolution changes enable proper type resolution without linker-induced duplicates. ENG-21411 requests fixing optional peer resolution in the isolated linker; the modifications to isolated_install.zig, lockfile.zig, and Tree.zig implement exactly this fix by removing early exclusion of optional peers and handling their resolution. The test suite validates these scenarios work correctly. | ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/install/lockfile/Tree.zig(9 hunks)test/cli/install/isolated-install.test.ts(1 hunks)test/cli/install/registry/packages/one-one-dep/package.json(1 hunks)test/cli/install/registry/packages/one-optional-peer-dep/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cli/install/isolated-install.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/registry/packages/one-one-dep/package.jsontest/cli/install/registry/packages/one-optional-peer-dep/package.json
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/lockfile/Tree.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/install/lockfile/Tree.zig
🧠 Learnings (1)
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/cli/install/registry/packages/one-one-dep/package.json
🔇 Additional comments (8)
src/install/lockfile/Tree.zig (8)
52-69: LGTM: Well-structured union for resolution outcomes.The expanded
HoistDependencyResultclearly differentiates between resolution pathways (.resolve,.resolve_replace,.resolve_later) and placement (.placement), making the optional peer resolution logic explicit and maintainable.
338-344: LGTM: Allows unresolved optional peers to participate in hoisting.By returning
false(not filtered) for optional peers with invalid package IDs, this ensures they can still resolve to existing packages during the hoisting process, which aligns with the PR objective of fixing optional peer resolution.
505-509: LGTM: Correctly skips unresolved packages during filtered builds.The guard ensures that during filtered installs (
.filtermethod), unresolved packages are skipped since they've already had their chance to resolve in the initial pass.
537-553: LGTM: Explicit handling of unresolved optional peers.This logic correctly:
- Attempts to hoist unresolved optional peers (allowing them to resolve to existing packages)
- Skips other unresolvable dependencies
This is the key fix preventing optional peers from being prematurely excluded.
707-713: LGTM: Peer version satisfaction allows hoisting.The logic correctly checks if an existing peer satisfies the version requirement of a new peer dependency, allowing safe deduplication when versions are compatible.
669-687: Verify manually that optional peer edge cases are properly handled and tested.The search for test patterns matching optional peer scenarios returned no results, preventing confirmation that these edge cases are covered by tests. While the conditional logic in lines 669-687 appears syntactically sound (correctly routing invalid/valid ID combinations), the specific edge cases you raised—particularly mixed optional/required peer designations and version constraint conflicts—require verification against:
- The full
hoistDependencyfunction implementation to see how edge cases are resolved- Existing test suites (the search patterns may not capture all related tests)
- Real-world scenarios in the lockfile to confirm expected behavior
574-623: Now I need to look deeper into the test implementation and the code to determine if the concerns are actually validated:Based on my investigation, I found that Bun has had issues with optional peer dependency resolution, and Bun install handles peer dependencies similarly to yarn, automatically installing them unless marked optional in peerDependenciesMeta.
The test output from the first script shows that tests for optional peers do exist in
test/cli/install/isolated-install.test.ts(lines 470-557), specifically testing scenarios with multiple packages (pkg1, pkg2) both depending on the same optional peer. However, I cannot definitively verify from the codebase search whether these tests specifically validate the name hash deduplication and propagation logic described in lines 581-588 and 593-600 of Tree.zig.The review comment's three verification points are legitimate concerns given:
- The complex state management in the resolution handling
- Historical bugs in Bun's peer dependency resolution
- The critical nature of correct propagation to avoid double installation or missed resolutions
Since I cannot confirm the specific coverage of these scenarios through the available tools, this warrants manual verification.
Verify that optional peer resolution propagation scenarios are properly tested.
The code at lines 581-588 (
.resolvecase) and 593-600 (.resolve_replacecase) propagates resolutions to pending optional peers sharing the samename_hash. While tests for optional peers with multiple importers exist intest/cli/install/isolated-install.test.ts, verify the following are validated:
- Multiple unresolved optional peers with the same name hash all resolve to the same package — confirm tests check that all peers in
pending_optional_peers[name_hash]receive the same resolution- Resolution propagation doesn't cross name hash boundaries — ensure tests don't show incorrect propagation to unrelated peers
- Pending peers list cleanup — confirm
fetchRemove()properly removes the entry after resolution anddefer peers.deinit()releases memory
627-636: Placement logic guard is correct; no issues found.Verification confirms the guard at line 627 is properly placed. Dependencies added to
dependency_listsat line 625 are queued for subtree processing only whenpkg_id != invalid_package_idandbuilder.resolution_lists[pkg_id].len > 0. This is correct because:
processSubtree(line 454) returns immediately ifresolution_list.len == 0, making the queue write unnecessary- Unqueued dependencies with
invalid_package_idremain in the tree structure, which is appropriate- Downstream code reading
dependency_listsentries explicitly handles invalid resolutions (e.g., line 667-669 inhoistDependency)- Pattern is consistent with the replacement branch (line 606)
| "dist": { | ||
| "integrity": "sha512-S6Bn6e9/9cZITQE2DiS8P3gd+Zwb6DF8LyLPcNemmE0nLh5kfjnxnGypmjzaRnSRnfqjEfqRFhjJPomjyNphuQ==", | ||
| "shasum": "52fab2c318e3e5b13fc6a4fb52da3791d79bba84", | ||
| "tarball": "http://http://localhost:4873/one-one-dep/-/one-one-dep-1.0.0.tgz" |
There was a problem hiding this comment.
Fix malformed tarball URL.
The tarball URL contains a duplicate protocol prefix http://http://, which will cause incorrect URL resolution during package installation.
Apply this diff to fix the URL:
- "tarball": "http://http://localhost:4873/one-one-dep/-/one-one-dep-1.0.0.tgz"
+ "tarball": "http://localhost:4873/one-one-dep/-/one-one-dep-1.0.0.tgz"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "tarball": "http://http://localhost:4873/one-one-dep/-/one-one-dep-1.0.0.tgz" | |
| "tarball": "http://localhost:4873/one-one-dep/-/one-one-dep-1.0.0.tgz" |
🤖 Prompt for AI Agents
In test/cli/install/registry/packages/one-one-dep/package.json around line 19,
the tarball URL is malformed with a duplicated protocol
("http://http://localhost:4873/..."); update the value to remove the extra
"http://" so the tarball field reads
"http://localhost:4873/one-one-dep/-/one-one-dep-1.0.0.tgz" (i.e., replace
"http://http://localhost:4873/..." with "http://localhost:4873/...").
| "dist": { | ||
| "integrity": "sha512-tqKLkZX69BL7dsv8ldPBeNqypOS2QxPEyqdB48kbcf0oyPGzLK+tOuDFEK7U63H+PoGErF+ro7reQUztRyL/pA==", | ||
| "shasum": "70ab2e73a16d86c1ed2547889025df30b8534c71", | ||
| "tarball": "http://http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.1.tgz" |
There was a problem hiding this comment.
Fix malformed tarball URL for version 1.0.1.
The tarball URL contains a duplicate protocol prefix http://http://, which will cause incorrect URL resolution during package installation.
Apply this diff to fix the URL:
- "tarball": "http://http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.1.tgz"
+ "tarball": "http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.1.tgz"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "tarball": "http://http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.1.tgz" | |
| "tarball": "http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.1.tgz" |
🤖 Prompt for AI Agents
In test/cli/install/registry/packages/one-optional-peer-dep/package.json around
line 19, the tarball URL is malformed with a duplicated protocol
"http://http://"; update the tarball value to use a valid URL by removing the
extra "http://" prefix so it becomes
"http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.1.tgz".
| "dist": { | ||
| "integrity": "sha512-S25U8/QXGIKfn/AWtsce1aVMnDjDL+ykFtAufpsuKGad32NlsCpi9TDuXvzoTQ+MdaZpGV3c4xghUZUsNeMp4A==", | ||
| "shasum": "0fbf084260c3decd365aaaf2d72526ec2d01fe1d", | ||
| "tarball": "http://http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.2.tgz" |
There was a problem hiding this comment.
Fix malformed tarball URL for version 1.0.2.
The tarball URL contains a duplicate protocol prefix http://http://, which will cause incorrect URL resolution during package installation.
Apply this diff to fix the URL:
- "tarball": "http://http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.2.tgz"
+ "tarball": "http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.2.tgz"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "tarball": "http://http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.2.tgz" | |
| "tarball": "http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.2.tgz" |
🤖 Prompt for AI Agents
In test/cli/install/registry/packages/one-optional-peer-dep/package.json around
line 43, the tarball URL for version 1.0.2 is malformed with a duplicated
protocol prefix ("http://http://"); update the tarball value to a valid URL by
removing the extra "http://" so it reads
"http://localhost:4873/one-optional-peer-dep/-/one-optional-peer-dep-1.0.2.tgz"
(ensure no extra spaces or quotes are altered).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/install/lockfile/Tree.zig (1)
575-624: Consider extracting duplicate peer resolution logic.The code correctly implements three resolution pathways for optional peers:
.resolve: Resolves the current dependency.resolve_replace: Replaces an existing unresolved dependency.resolve_later: Defers resolutionHowever, lines 582-589 and 594-601 contain duplicate logic for resolving pending optional peers with the same
name_hash.Consider extracting this into a helper method:
fn resolvePendingOptionalPeers( builder: *Builder(method), name_hash: PackageNameHash, resolved_pkg_id: PackageID, ) !void { if (builder.pending_optional_peers.fetchRemove(name_hash)) |entry| { var peers = entry.value; defer peers.deinit(); for (peers.items()) |unresolved_dep_id| { bun.assertWithLocation(builder.resolutions[unresolved_dep_id] == invalid_package_id, @src()); builder.resolutions[unresolved_dep_id] = resolved_pkg_id; } } }Then call it from both locations:
.resolve => |res_id| { bun.assertWithLocation(pkg_id == invalid_package_id, @src()); bun.assertWithLocation(res_id != invalid_package_id, @src()); builder.resolutions[dep_id] = res_id; if (comptime Environment.allow_assert) { bun.assertWithLocation(!builder.pending_optional_peers.contains(dependency.name_hash), @src()); } - if (builder.pending_optional_peers.fetchRemove(dependency.name_hash)) |entry| { - var peers = entry.value; - defer peers.deinit(); - for (peers.items()) |unresolved_dep_id| { - bun.assertWithLocation(builder.resolutions[unresolved_dep_id] == invalid_package_id, @src()); - builder.resolutions[unresolved_dep_id] = res_id; - } - } + try resolvePendingOptionalPeers(builder, dependency.name_hash, res_id); }, .resolve_replace => |replace| { bun.assertWithLocation(pkg_id != invalid_package_id, @src()); builder.resolutions[replace.dep_id] = pkg_id; - if (builder.pending_optional_peers.fetchRemove(dependency.name_hash)) |entry| { - var peers = entry.value; - defer peers.deinit(); - for (peers.items()) |unresolved_dep_id| { - bun.assertWithLocation(builder.resolutions[unresolved_dep_id] == invalid_package_id, @src()); - builder.resolutions[unresolved_dep_id] = pkg_id; - } - } + try resolvePendingOptionalPeers(builder, dependency.name_hash, pkg_id);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/install/lockfile/Tree.zig(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/lockfile/Tree.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/install/lockfile/Tree.zig
🧠 Learnings (3)
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to src/**/*.zig : In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-15T22:03:50.832Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/envvars.zig:135-144
Timestamp: 2025-10-15T22:03:50.832Z
Learning: In src/envvars.zig, the boolean feature flag cache uses a single atomic enum and should remain monotonic. Only the string cache (which uses two atomics: ptr and len) requires acquire/release ordering to prevent torn reads.
Applied to files:
src/install/lockfile/Tree.zig
🔇 Additional comments (11)
src/install/lockfile/Tree.zig (11)
52-69: LGTM! Well-structured union for resolution pathways.The expanded
HoistDependencyResultunion cleanly represents the different outcomes of hoisting dependencies, with explicit variants for resolving optional peers (.resolve,.resolve_replace,.resolve_later) and placement (.placement). The inner structs are appropriately scoped.
244-244: LGTM! Mutability enables dynamic resolution updates.Changing
resolutionsfrom[]const PackageIDto[]PackageIDis intentional to support updating resolutions when optional peers are resolved later in the hoisting process.
250-252: LGTM! Clear documentation for pending optional peers.The comment clearly explains the purpose of
pending_optional_peersmap for tracking unresolved optional peers that may resolve later.
313-313: LGTM! Cleanup properly implemented.The
pending_optional_peers.deinit()call correctly releases the AutoHashMap resources in theclean()method, addressing the memory leak flagged in the previous review.
339-345: LGTM! Correct handling of unresolved optional peers.The logic correctly prevents filtering out optional peers when they haven't resolved yet (
pkg_id >= lockfile.packages.len). This gives optional peers a chance to resolve to existing packages later, aligning with the PR's objective to fix optional peer resolution.
506-510: LGTM! Correct filtering of unresolved packages.When filtering, unresolved packages (
invalid_package_id) are correctly skipped since they already had their chance to resolve earlier in the process.
538-554: LGTM! Proper handling of unresolved optional peers.The logic correctly allows optional peers with
invalid_package_idto attempt hoisting (lines 539-550), which may result in them resolving to an existing package. Non-optional unresolved dependencies are correctly skipped (lines 552-554).
628-628: LGTM! Proper guard against invalid packages.The check
pkg_id != invalid_package_idcorrectly prevents queuing subtree processing for unresolved packages.
668-693: LGTM! Comprehensive optional peer resolution logic.The
hoistDependencyfunction correctly handles all combinations of valid/invalid package IDs:
- Both invalid (lines 670-676): Two unresolved optional peers with the same name → defer resolution (
.resolve_later)- Existing invalid (lines 678-681): Replace the existing unresolved peer with the new resolved one (
.resolve_replace)- New invalid (lines 683-688): Resolve the new optional peer to the existing resolved package (
.resolve)- Both valid and equal (lines 690-693): Standard deduplication (
.hoisted)The assertions properly verify that invalid packages are always optional peers, ensuring type safety.
709-709: LGTM! Correct use of resolved package ID.Using
res_id(the actual resolved package) instead ofpackage_id(which may beinvalid_package_idfor optional peers) is correct for checking version compatibility.
728-729: LGTM! Consistent use of resolved package ID.Using
res_idin error messages ensures accurate reporting of the actual resolved package involved in dependency loops.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
test/cli/install/__snapshots__/migrate-bun-lockb-v2.test.ts.snapis excluded by!**/*.snaptest/integration/next-pages/test/__snapshots__/dev-server-ssr-100.test.ts.snapis excluded by!**/*.snaptest/integration/next-pages/test/__snapshots__/dev-server.test.ts.snapis excluded by!**/*.snaptest/integration/next-pages/test/__snapshots__/next-build.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
test/cli/install/migrate-bun-lockb-v2.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/migrate-bun-lockb-v2.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/install/migrate-bun-lockb-v2.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/install/migrate-bun-lockb-v2.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/cli/install/migrate-bun-lockb-v2.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/cli/install/migrate-bun-lockb-v2.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or invent random port functions
Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Never write tests that assert absence of crashes (e.g., no "panic" or "uncaught exception") in output
Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
When spawning processes in tests, assert on stdout before asserting exitCode
Do not use setTimeout in tests; await conditions instead to avoid flakiness
Files:
test/cli/install/migrate-bun-lockb-v2.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Files:
test/cli/install/migrate-bun-lockb-v2.test.ts
🧠 Learnings (18)
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.{ts,tsx} : Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/ecosystem.test.ts : ecosystem.test.ts should focus on concrete library integration bugs rather than whole-package coverage
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Explicitly assert client console logs with c.expectMessage; unasserted logs should not appear
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/plugins.test.ts : plugins.test.ts should contain plugin-related development-mode tests
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/esm.test.ts : esm.test.ts should cover ESM feature behavior in development mode
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.test.{ts,tsx} : When spawning processes in tests, assert on stdout before asserting exitCode
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Always check exit codes and error scenarios in tests (e.g., spawned processes should assert non-zero on failure)
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/cli/install/migrate-bun-lockb-v2.test.ts
🔇 Additional comments (1)
test/cli/install/migrate-bun-lockb-v2.test.ts (1)
109-124: LGTM: Stability verification is correct.The idempotency check correctly verifies that a second
bun installproduces identical lockfile bytes and parsed content, ensuring migration stability.
| // contents should be different due to semver numbers changing size | ||
| expect(newLockfileContents).not.toEqual(oldLockfileContents); | ||
| // but parse result should be the same | ||
| expect(newLockfile).toEqual(oldLockfile); | ||
| expect(newLockfile).toMatchSnapshot(); |
There was a problem hiding this comment.
Update comment and consider using normalizeBunSnapshot().
The comment on line 106 states "but parse result should be the same," but the assertion no longer verifies equivalence between the old and new parsed lockfiles—it only snapshots the new one. Additionally, per coding guidelines, snapshot assertions should use normalizeBunSnapshot() to ensure stable, normalized output.
Concerns:
- Misleading comment: The comment implies semantic equivalence is verified, but it's not.
- Missing normalization: Snapshots should be normalized to avoid flakiness.
- Loss of migration verification: The test no longer confirms that migration preserves the semantic content of the old lockfile. If lockfile structure legitimately changed (e.g., due to new optional peer resolution), document why direct comparison isn't feasible.
Consider this approach:
- // contents should be different due to semver numbers changing size
expect(newLockfileContents).not.toEqual(oldLockfileContents);
- // but parse result should be the same
- expect(newLockfile).toMatchSnapshot();
+ // Verify migrated lockfile structure matches expected snapshot
+ expect(normalizeBunSnapshot(newLockfile)).toMatchSnapshot();If semantic equivalence with the old lockfile is no longer expected due to PR changes (new resolution paths, pending_optional_peers), add a comment explaining why.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // contents should be different due to semver numbers changing size | |
| expect(newLockfileContents).not.toEqual(oldLockfileContents); | |
| // but parse result should be the same | |
| expect(newLockfile).toEqual(oldLockfile); | |
| expect(newLockfile).toMatchSnapshot(); | |
| expect(newLockfileContents).not.toEqual(oldLockfileContents); | |
| // Verify migrated lockfile structure matches expected snapshot | |
| expect(normalizeBunSnapshot(newLockfile)).toMatchSnapshot(); |
🤖 Prompt for AI Agents
In test/cli/install/migrate-bun-lockb-v2.test.ts around lines 104 to 107, the
comment claims the parsed lockfile results should be the same but the test only
snapshots the new parse and doesn't normalize it; update the comment to reflect
actual expectation (either that semantic equivalence is required or that changes
are expected and why), and then either (a) assert semantic equivalence by
comparing the parsed old and new lockfile objects (e.g.,
expect(newLockfile).toEqual(oldLockfile) or a deep-equals), or (b) if
equivalence is no longer expected, add a clear comment listing the expected
differences and why, and change the snapshot assertion to use
normalizeBunSnapshot(newLockfile) so the snapshot is stable.
|
Thanks @dylan-conway, super happy this got some attention. Any there will be a minor patch release that includes this in short notice? I'll try this out tomorrow by upgrading our workflows to the latest canary. |
|
@RobbyUitbeijerse yes we're planning to do a patch release in the first half of this week. Let me know if you run into any issues trying this fix in canary/v1.3.2 |
|
Sounds good, thank you! I just managed to test the latest canary of bun and can confirm that the issue(s) we were having are resolved both on my machine as well as in our Github workflows and in Vercel. |
What does this PR do?
Allows optional peers to resolve to package if possible.
Optional peers aren't auto-installed, but they should still be given a chance to resolve. If they're always left unresolved it's possible for multiple dependencies on the same package to result in different peer resolutions when they should be the same. For example, this bug this could cause monorepos using elysia to have corrupt node_modules because there might be more than one copy of elysia in
node_modules/.bun(or more than the expected number of copies).fixes #23725
most likely fixes #23895
fixes ENG-21411
How did you verify your code works?
Added a test for optional peers and non-optional peers that would previously trigger this bug.
Summary by CodeRabbit
New Features
Tests