Repository navigation
add support for "workspaces.nohoist" - #20178
dylan-conway wants to merge 22 commits into
Conversation
|
Waiting for this!!!!! |
|
Also looking forward to this! |
|
@dylan-conway Could you please itemize what needs doing on this, so the community can help out if possible? Looks like there are a few people in here who need this urgently and might want to help out. Thanks for the great work! |
|
@robbyemmert The remaining unchecked TODO tests in the description is a pretty good summary of the remaining work. Most of these tests should already be passing with the existing code, they just need to be added to a test file (like I'm planning to continue work on this soon. We decided to complete |
|
@dylan-conway I'm very happy that hoisting settings are under way! With the newly released symlink install I thought I'd chime in because it allows for the more practical pnpm hoisting semantics, which aren't possible with the traditional node_modules linker. People generally want one of 3 things: B is what most people want, because you want to prevent dependencies not under your control from breaking, while avoiding ghost dependencies in your own application code. Yarn-style hoisting controls with
|
|
Updated 5:47 PM PT - Nov 14th, 2025
❌ @dylan-conway, your commit 16de926 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 20178That installs a local version of the PR into your bun-20178 --bun |
dylan-conway
changed the title
"workspaces.nohoist" and "workspaces.hoistingLimits""workspaces.nohoist"
a42f4cf to
2d02d7f
Compare
WalkthroughAdds workspace "nohoist" support: stores nohoist patterns on PackageManager, parses Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/lockfile/Tree.zig (1)
514-705: Releasedep_subpathwhen it doesn’t escapeInside the hoist loop we reserve
dep_subpathfor every dependency whenevernohoist_patternsis non-empty. When the hoist result is.hoisted,.resolve,.resolve_later, or even.placementwithout requeue, wecontinuewithout ever callingdeinit(), so those buffers leak per dependency processed. This regresses long-lived installs pretty quickly on large workspaces.Please make the subpath buffer RAII-safe—e.g.
var dep_subpath = .init(); var moved = false; defer if (comptime method == .filter and !moved) dep_subpath.deinit();—and flipmoved = trueonly when you hand the buffer to the queue. That way we still transfer ownership for recursive processing, but we always free allocations that stay local to this frame.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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 (6)
src/install/PackageManager.zig(1 hunks)src/install/lockfile.zig(1 hunks)src/install/lockfile/Package.zig(1 hunks)src/install/lockfile/Tree.zig(8 hunks)test/cli/install/nohoist.test.ts(1 hunks)test/harness.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/nohoist.test.tstest/harness.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/nohoist.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/nohoist.test.tstest/harness.ts
**/*.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.zigsrc/install/PackageManager.zigsrc/install/lockfile/Tree.zigsrc/install/lockfile/Package.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)
Files:
src/install/lockfile.zigsrc/install/PackageManager.zigsrc/install/lockfile/Tree.zigsrc/install/lockfile/Package.zig
🧠 Learnings (23)
📚 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/nohoist.test.tstest/harness.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/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/cli/install/nohoist.test.tstest/harness.tssrc/install/lockfile/Tree.zig
📚 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} : Place CLI command tests (e.g., bun install, bun init) under test/cli/
Applied to files:
test/cli/install/nohoist.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/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/cli/install/nohoist.test.tstest/harness.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/nohoist.test.tstest/harness.tssrc/install/lockfile/Tree.zig
📚 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/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/cli/install/nohoist.test.tstest/harness.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/nohoist.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/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/cli/install/nohoist.test.tstest/harness.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/js/third_party/**/*.{js,ts} : Place third-party npm package tests under test/js/third_party/
Applied to files:
test/cli/install/nohoist.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/nohoist.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/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/cli/install/nohoist.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/cli/install/nohoist.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 : Write Dev Server and HMR tests in test/bake/dev/*.test.ts using devTest from the shared harness
Applied to files:
test/cli/install/nohoist.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} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts
Applied to files:
test/cli/install/nohoist.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/harness.tssrc/install/lockfile/Tree.zig
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
Applied to files:
test/harness.tssrc/install/lockfile/Tree.zig
📚 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/harness.ts
📚 Learning: 2025-10-01T22:13:08.081Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/codegen/bindgenv2/internal/base.ts:120-125
Timestamp: 2025-10-01T22:13:08.081Z
Learning: Iterator.some() and other Iterator helper methods (map, filter, etc.) are supported in Bun and modern Node.js (22+) runtimes and can be safely used on string iterators and other iterator objects.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-08T13:56:00.875Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-08T03:11:45.286Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-10-08T03:11:45.286Z
Learning: Applies to src/**/*.zig : When adding debug logs in Zig, create a scoped logger and log via Bun APIs: `const log = bun.Output.scoped(.${SCOPE}, .hidden);` then `log("...", .{})`
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/install/lockfile/Tree.zig
📚 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/V8*.h : Ensure all public V8 API functions are marked with BUN_EXPORT for symbol visibility
Applied to files:
src/install/lockfile/Tree.zig
🧬 Code graph analysis (1)
test/cli/install/nohoist.test.ts (1)
test/harness.ts (3)
VerdaccioRegistry(1636-1777)runBunInstall(1215-1275)readdirSorted(1786-1790)
🔇 Additional comments (3)
test/cli/install/nohoist.test.ts (1)
1-14: LGTM!The test setup properly uses the VerdaccioRegistry harness with correct lifecycle management in beforeAll/afterAll hooks.
src/install/PackageManager.zig (2)
138-139: No action needed —nohoist_patternsfield is already properly initialized.The field is populated during
PackageManagerinitialization when parsing theworkspaces.nohoistconfiguration from package.json (src/install/lockfile/Package.zig:1590–1601). A temporary ArrayList collects and validates pattern strings, then the populated collection is converted to a slice and assigned to the field.Likely an incorrect or invalid review comment.
141-158: Review comment is incorrect—PackageManager.zig was not modified in this PR.The
Subcommandenum expansion you flagged is pre-existing code in the repository. The filesrc/install/PackageManager.zigdoes not appear in the list of 51 files modified by this PR, and git diff shows no changes to it. The code snippet shown is from the current state of the file before any PR changes, not a newly added or modified section. Therefore, concerns about scope creep or unrelated changes to this enum are not applicable.Likely an incorrect or invalid review comment.
| describe("workspaces.nohoist", () => { | ||
| test("basic", async () => { | ||
| const { packageDir } = await registry.createTestDir({ | ||
| files: { | ||
| "package.json": JSON.stringify({ | ||
| name: "basic-nohoist", | ||
| workspaces: { | ||
| nohoist: ["one-dep/no-deps"], | ||
| }, | ||
| dependencies: { | ||
| "one-dep": "1.0.0", | ||
| }, | ||
| }), | ||
| }, | ||
| }); | ||
|
|
||
| await runBunInstall(bunEnv, packageDir); | ||
|
|
||
| expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["one-dep"]); | ||
| }); | ||
|
|
||
| test("can keep package in workspace node_modules", async () => { | ||
| const { packageDir } = await registry.createTestDir({ | ||
| files: { | ||
| "package.json": JSON.stringify({ | ||
| name: "workspace-nohoist", | ||
| workspaces: { | ||
| packages: ["packages/*"], | ||
| nohoist: ["**/one-dep"], | ||
| }, | ||
| dependencies: { | ||
| "a-dep": "1.0.1", | ||
| }, | ||
| }), | ||
| "packages/pkg1/package.json": JSON.stringify({ | ||
| name: "pkg1", | ||
| dependencies: { | ||
| "one-dep": "1.0.0", | ||
| }, | ||
| }), | ||
| }, | ||
| }); | ||
|
|
||
| await runBunInstall(bunEnv, packageDir, { linker: "hoisted" }); | ||
|
|
||
| expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["a-dep", "no-deps", "pkg1"]); | ||
| expect(await readdirSorted(join(packageDir, "packages/pkg1/node_modules"))).toEqual(["one-dep"]); | ||
| }); | ||
|
|
||
| test("handles cycles", async () => { | ||
| const { packageDir } = await registry.createTestDir({ | ||
| files: { | ||
| "package.json": JSON.stringify({ | ||
| name: "cycles", | ||
| workspaces: { | ||
| nohoist: ["**"], | ||
| }, | ||
| dependencies: { | ||
| "a-dep-b": "1.0.0", | ||
| }, | ||
| }), | ||
| }, | ||
| }); | ||
|
|
||
| await runBunInstall(bunEnv, packageDir, { linker: "hoisted" }); | ||
|
|
||
| expect( | ||
| await Promise.all([ | ||
| readdirSorted(join(packageDir, "node_modules")), | ||
| readdirSorted(join(packageDir, "node_modules/a-dep-b/node_modules")), | ||
| exists(join(packageDir, "node_modules/a-dep-b/node_modules/b-dep-a/node_modules")), | ||
| ]), | ||
| ).toEqual([["a-dep-b"], ["b-dep-a"], false]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add remaining test cases per PR objectives.
The PR objectives mention a TODO checklist that includes additional test scenarios. Current test coverage includes basic, workspace, and cycle cases, but consider adding:
- Tests for invalid nohoist patterns
- Tests with
linker: "isolated" - Tests for root package nohoist patterns
- Tests matching all packages
- Error handling cases
These additional tests would help reveal remaining bugs as mentioned in the PR description.
Would you like me to help generate additional test cases to cover these scenarios?
🤖 Prompt for AI Agents
In test/cli/install/nohoist.test.ts around lines 16 to 90, add the remaining
test cases referenced in the PR TODO: create new tests for invalid nohoist
patterns (assert install fails or emits specific error), run at least one
install with linker: "isolated" to verify nohoist behavior under isolated
linking, add a test for root-package nohoist patterns (nohoist entries that
target the root and assert correct placement), add a test that uses a pattern
matching all packages (e.g., "**") to confirm global nohoist behavior, and add
error-handling tests (malformed package.json or conflicting nohoist entries)
that assert proper error messages; implement each test following the existing
pattern: use registry.createTestDir to create fixture files, call runBunInstall
with the proper bunEnv and linker option, then assert node_modules layout or
error output using readdirSorted/exists/expect.
| test("basic", async () => { | ||
| const { packageDir } = await registry.createTestDir({ | ||
| files: { | ||
| "package.json": JSON.stringify({ | ||
| name: "basic-nohoist", | ||
| workspaces: { | ||
| nohoist: ["one-dep/no-deps"], | ||
| }, | ||
| dependencies: { | ||
| "one-dep": "1.0.0", | ||
| }, | ||
| }), | ||
| }, | ||
| }); | ||
|
|
||
| await runBunInstall(bunEnv, packageDir); | ||
|
|
||
| expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["one-dep"]); | ||
| }); |
There was a problem hiding this comment.
Incomplete test validation - verify nohoist behavior.
The test configures nohoist: ["one-dep/no-deps"] but only checks that "one-dep" exists in the root node_modules. It doesn't verify the actual nohoist behavior - that "no-deps" is kept nested within "one-dep/node_modules" instead of being hoisted to the root.
Apply this diff to add the missing assertion:
await runBunInstall(bunEnv, packageDir);
expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["one-dep"]);
+ expect(await readdirSorted(join(packageDir, "node_modules/one-dep/node_modules"))).toEqual(["no-deps"]);🤖 Prompt for AI Agents
In test/cli/install/nohoist.test.ts around lines 17 to 35, the test only asserts
that "one-dep" exists at the root but doesn't verify nohoist semantics for
"one-dep/no-deps"; update the test to assert that "no-deps" is present under
join(packageDir, "node_modules", "one-dep", "node_modules") and that "no-deps"
is not present in the root node_modules (i.e., ensure
readdirSorted(join(packageDir, "node_modules", "one-dep", "node_modules"))
includes "no-deps" and readdirSorted(join(packageDir, "node_modules")) does not
include "no-deps").
| test("can keep package in workspace node_modules", async () => { | ||
| const { packageDir } = await registry.createTestDir({ | ||
| files: { | ||
| "package.json": JSON.stringify({ | ||
| name: "workspace-nohoist", | ||
| workspaces: { | ||
| packages: ["packages/*"], | ||
| nohoist: ["**/one-dep"], | ||
| }, | ||
| dependencies: { | ||
| "a-dep": "1.0.1", | ||
| }, | ||
| }), | ||
| "packages/pkg1/package.json": JSON.stringify({ | ||
| name: "pkg1", | ||
| dependencies: { | ||
| "one-dep": "1.0.0", | ||
| }, | ||
| }), | ||
| }, | ||
| }); | ||
|
|
||
| await runBunInstall(bunEnv, packageDir, { linker: "hoisted" }); | ||
|
|
||
| expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["a-dep", "no-deps", "pkg1"]); | ||
| expect(await readdirSorted(join(packageDir, "packages/pkg1/node_modules"))).toEqual(["one-dep"]); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Request clarification on test package dependencies.
The test expects "no-deps" to appear in the root node_modules, but it's not declared as a direct dependency. Please add a comment explaining the dependency graph (e.g., whether "no-deps" is a transitive dependency of "a-dep" or "one-dep") to make the test expectations clearer.
Example:
test("can keep package in workspace node_modules", async () => {
+ // Dependency graph: a-dep@1.0.1 -> no-deps, pkg1 -> one-dep@1.0.0 -> no-deps
+ // nohoist pattern "**/one-dep" keeps one-dep in workspace node_modules
const { packageDir } = await registry.createTestDir({📝 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.
| test("can keep package in workspace node_modules", async () => { | |
| const { packageDir } = await registry.createTestDir({ | |
| files: { | |
| "package.json": JSON.stringify({ | |
| name: "workspace-nohoist", | |
| workspaces: { | |
| packages: ["packages/*"], | |
| nohoist: ["**/one-dep"], | |
| }, | |
| dependencies: { | |
| "a-dep": "1.0.1", | |
| }, | |
| }), | |
| "packages/pkg1/package.json": JSON.stringify({ | |
| name: "pkg1", | |
| dependencies: { | |
| "one-dep": "1.0.0", | |
| }, | |
| }), | |
| }, | |
| }); | |
| await runBunInstall(bunEnv, packageDir, { linker: "hoisted" }); | |
| expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["a-dep", "no-deps", "pkg1"]); | |
| expect(await readdirSorted(join(packageDir, "packages/pkg1/node_modules"))).toEqual(["one-dep"]); | |
| }); | |
| test("can keep package in workspace node_modules", async () => { | |
| // Dependency graph: a-dep@1.0.1 -> no-deps, pkg1 -> one-dep@1.0.0 -> no-deps | |
| // nohoist pattern "**/one-dep" keeps one-dep in workspace node_modules | |
| const { packageDir } = await registry.createTestDir({ | |
| files: { | |
| "package.json": JSON.stringify({ | |
| name: "workspace-nohoist", | |
| workspaces: { | |
| packages: ["packages/*"], | |
| nohoist: ["**/one-dep"], | |
| }, | |
| dependencies: { | |
| "a-dep": "1.0.1", | |
| }, | |
| }), | |
| "packages/pkg1/package.json": JSON.stringify({ | |
| name: "pkg1", | |
| dependencies: { | |
| "one-dep": "1.0.0", | |
| }, | |
| }), | |
| }, | |
| }); | |
| await runBunInstall(bunEnv, packageDir, { linker: "hoisted" }); | |
| expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["a-dep", "no-deps", "pkg1"]); | |
| expect(await readdirSorted(join(packageDir, "packages/pkg1/node_modules"))).toEqual(["one-dep"]); | |
| }); |
🤖 Prompt for AI Agents
test/cli/install/nohoist.test.ts around lines 37 to 63: the test asserts that
"no-deps" is present in the root node_modules but the test data doesn't document
why that package appears; add an inline comment above the registry.createTestDir
call (or directly above the expectations) that explains the dependency
graph—specifically state which package (a-dep or one-dep) depends on "no-deps"
transitively and therefore why it should be hoisted into the root
node_modules—so future readers understand why the expectation includes
"no-deps".
| test("handles cycles", async () => { | ||
| const { packageDir } = await registry.createTestDir({ | ||
| files: { | ||
| "package.json": JSON.stringify({ | ||
| name: "cycles", | ||
| workspaces: { | ||
| nohoist: ["**"], | ||
| }, | ||
| dependencies: { | ||
| "a-dep-b": "1.0.0", | ||
| }, | ||
| }), | ||
| }, | ||
| }); | ||
|
|
||
| await runBunInstall(bunEnv, packageDir, { linker: "hoisted" }); | ||
|
|
||
| expect( | ||
| await Promise.all([ | ||
| readdirSorted(join(packageDir, "node_modules")), | ||
| readdirSorted(join(packageDir, "node_modules/a-dep-b/node_modules")), | ||
| exists(join(packageDir, "node_modules/a-dep-b/node_modules/b-dep-a/node_modules")), | ||
| ]), | ||
| ).toEqual([["a-dep-b"], ["b-dep-a"], false]); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good edge case coverage for circular dependencies.
The test correctly validates nohoist behavior with circular dependencies using the "**" pattern. Consider adding a comment explaining the circular dependency structure for better test documentation.
Example:
test("handles cycles", async () => {
+ // Tests circular dependency: a-dep-b -> b-dep-a -> a-dep-b
+ // nohoist "**" prevents hoisting, but cycle must be broken to avoid infinite nesting
const { packageDir } = await registry.createTestDir({🤖 Prompt for AI Agents
In test/cli/install/nohoist.test.ts around lines 65 to 89, add a short
explanatory comment above the "handles cycles" test that documents the circular
dependency topology being exercised (e.g., package depends on a-dep-b, a-dep-b
pulls in b-dep-a, and b-dep-a creates the cycle) and why the "**" nohoist
pattern is used; keep the comment concise (1–2 sentences) and place it directly
above the test block so future readers understand the edge case being validated.
| const dependency = builder.dependencies[dep_id]; | ||
|
|
||
| const hoisted: HoistDependencyResult = hoisted: { | ||
| if (comptime method == .filter) { |
There was a problem hiding this comment.
This function is already 711 lines long. I would suggest taking your changes and putting them in a function which answers a self-contained question.
Here's how I would approach refactoring this:
- What are the different intents that this function has?
a) Are there intents that seem self-contained?
I can identify two chunks of code that have two pretty self-contained intents:
- The loop which checks whether something matches a no-hoist pattern without allowing circular dependencies.
- The loop which answers whether something is circular or not.
Refactor 2.
Let's start with 2:
var curr = trees[next.id].parent;
while (curr != invalid_id) {
const curr_dep_id = trees[curr].dependency_id;
const curr_pkg_id = switch (curr_dep_id) {
root_dep_id => 0,
else => builder.resolutions[curr_dep_id],
};
var curr_resolutions = builder.resolution_lists[curr_pkg_id];
for (curr_resolutions.begin()..curr_resolutions.end()) |tree_dep_id| {
const res_id = builder.resolutions[tree_dep_id];
if (res_id == pkg_id) {
break :try_nohoist;
}
}
curr = trees[curr].parent;
}This chunk of code is prefixed by a comment:
// make sure it's not circularLet's use that as the name:
fn makeSureTreeNotCircular(...) boolOkay, well this is a predicate, so maybe the verb should be is, rather than make:
fn isTreeCircular(...) boolGreat! That seems a little neater. What are the parameters that this function has? Now that we have the name, we can probably infer that it will have to take a tree. It shouldn't need to mutate it, so I suppose that it should take an immutable ref.
fn isTreeCircular(tree: *const Tree) boolGreat, let's copy-paste the implementation
fn isTreeCircular(tree: *const Tree) bool {
var curr = trees[next.id].parent;
while (curr != invalid_id) {
const curr_dep_id = trees[curr].dependency_id;
const curr_pkg_id = switch (curr_dep_id) {
root_dep_id => 0,
else => builder.resolutions[curr_dep_id],
};
var curr_resolutions = builder.resolution_lists[curr_pkg_id];
for (curr_resolutions.begin()..curr_resolutions.end()) |tree_dep_id| {
const res_id = builder.resolutions[tree_dep_id];
if (res_id == pkg_id) {
break :try_nohoist;
}
}
curr = trees[curr].parent;
}
}Wow! Far lower nesting, but this won't really compile as-is. There's a couple invalid references now. Without any compilation I see: trees, next and pkg_id. Let's change our signature, rename some variables, and replace the breaks with returns:
fn isTreeCircular(pkg_id: PackageID, forest: []const Tree, tree: *const Tree) bool {
var curr = forest[tree.id].parent;
while (curr != invalid_id) {
const curr_dep_id = forest[curr].dependency_id;
const curr_pkg_id = switch (curr_dep_id) {
root_dep_id => 0,
else => builder.resolutions[curr_dep_id],
};
var curr_resolutions = builder.resolution_lists[curr_pkg_id];
for (curr_resolutions.begin()..curr_resolutions.end()) |tree_dep_id| {
const res_id = builder.resolutions[tree_dep_id];
if (res_id == pkg_id) {
return true;
}
}
curr = forest[curr].parent;
}
return false;
}We still depend on builder, and the reason I haven't talked about it now is that there's a decision to be made. Do we:
a) Make this new function a member of builder?
b) Inject builder into the function as a dependency?
I lean towards the former. I ended up, after a little bit of cleanup and renaming on the following:
for (builder.manager.nohoist_patterns) |nohoist_pattern| {
if (glob.match(nohoist_pattern, dep_subpath.items()).matches()) {
if (builder.pkgHasCircularReference(pkg_id, trees, next)) {
break :try_nohoist;
}
break :hoisted .{ .placement = .{ .id = next.id } };
}
}Nesting this deep is usually a code-smell, but we have a cheap way to invert the condition and reduce one level of nesting.
for (builder.manager.nohoist_patterns) |nohoist_pattern| {
if (!glob.match(nohoist_pattern, dep_subpath.items()).matches()) {
continue;
}
if (builder.pkgHasCircularReference(pkg_id, trees, next)) {
break :try_nohoist;
}
break :hoisted .{ .placement = .{ .id = next.id } };
}Aside
This is very optional, but I would split up the loop, too:
for (builder.manager.nohoist_patterns) |nohoist_pattern| {
if (!glob.match(nohoist_pattern, dep_subpath.items()).matches()) {
continue;
}
if (builder.pkgHasCircularReference(pkg_id, trees, next)) {
break :try_nohoist;
}
break :hoisted .{ .placement = .{ .id = next.id } };
}Like so:
struct Builder {
fn shouldNoHoist(...)
};
and
```zig
if (builder.shouldNoHoist(dep_subpath.items(), pkg_id, trees, next) catch |err| {
switch (err) {
error.CircularDependency => break :try_nohoist,
}
}) {
break :hoisted .{ .placement = .{ .id = next.id } };
}Refactor 1
The next thing I would do is try to isolate the changes you've made into a self-contained function.
Blocks are crappier, weaker versions of functions, and you already have a pretty good name for a function here -- tryNoHoist, so let's stick with that.
You could maybe attach this to Builder, like so:
pub fn tryNohoist(
self: *const @This(),
subpath: []const u8,
dependency_id: DependencyID,
) ?error{CircularDependency}!struct {
result: HoistDependencyResult,
subpath: bun.collections.ArrayListDefault(u8),
}and you end up with something like this
if (try builder.tryNohoist(if (method == .filter) subpath.items() else {}, dep_id)) |r| {
dep_subpath = r.subpath;
break :hoisted r.result;
}At the call site.
All my suggestions are available here: #24646
| const dependency = builder.dependencies[dep_id]; | ||
| const pkg_id = builder.resolutions[dep_id]; | ||
|
|
||
| var dep_subpath: bun.collections.ArrayListDefault(u8) = .init(); |
There was a problem hiding this comment.
Why ArrayListDefault instead of StringBuilder? Seems like the latter is ideal for this use-case
| } | ||
| dep_subpath.appendSliceAssumeCapacity(dependency.name.slice(string_buf)); | ||
|
|
||
| if (dependency.version.tag == .workspace) { |
There was a problem hiding this comment.
Couple questions from me:
- Why are we excluding workspace dependencies, because they're not hoistable? If that's the case, should we not do the same for catalog dependencies?
- The coupling here is fragile. What would happen if we added a new
Tagthat may be incompatible with no-hoisting? I would suggest you add a new method to
pub const Tag = enum(u8) {
pub fn supportsHoisting(self: @This()) {
return switch(self) {
.uninitialized, workspace, catalog => false,
.npm, .dist_tag, ... => true,
};
}I would suggest against using else in this switch since you want additions/removals to Tag enum members to break compilation and cause us to look at as many potential breakage points as possible.
There was a problem hiding this comment.
There's a unique property to workspace dependencies in bun: they always have an implicit dependency appended to lockfile.buffers.dependencies, even if no packages depend on them. This means we need to always hoist workspace dependencies to make sure the implicit and explicit dependencies deduplicate correctly (otherwise there will be collisions due to duplicate names when we are linking node_modules)
Always hoisting them works because if the explicit workspace dependency name has a different name than the implicit workspace dependency it won't be able to hoist anyways matching the behavior of "nohoist"
| if (subpath.items().len != 0) { | ||
| dep_subpath.appendAssumeCapacity('/'); | ||
| } | ||
| dep_subpath.appendSliceAssumeCapacity(dependency.name.slice(string_buf)); |
There was a problem hiding this comment.
I don't completely understand dependency.name.slice(string_buf). Is it safe to use string_buf. How do you guarantee there's no useful data you're stomping over?
There was a problem hiding this comment.
string_buf is safe to use because at this stage we've finished downloading manifests and building packages in memory. It's during appending to lockfile.packages that lockfile.buffers.string_bytes.items is changed mostly
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/install/lockfile/Tree.zig (6)
267-267: Add documentation for the subpath fieldThe purpose and semantics of
subpathare not immediately clear. Please add a doc comment explaining what it represents (e.g., the accumulated path from the workspace root to the current dependency for nohoist pattern matching).Based on past review comments.
516-516: Consider using StringBuilder for path construction
StringBuilderis purpose-built for string concatenation and might be more appropriate thanArrayListDefault(u8)for buildingdep_subpath. It provides a clearer intent and potentially better ergonomics for path construction.Based on past review comments.
560-565: Simplify repeated condition checksThe condition
subpath.items().len != 0is evaluated twice, making the logic harder to follow. Consider restructuring:- try dep_subpath.ensureTotalCapacity(subpath.items().len + @intFromBool(subpath.items().len != 0) + dependency.name.len()); - dep_subpath.appendSliceAssumeCapacity(subpath.items()); - if (subpath.items().len != 0) { - dep_subpath.appendAssumeCapacity('/'); - } - dep_subpath.appendSliceAssumeCapacity(dependency.name.slice(string_buf)); + const subpath_items = subpath.items(); + if (subpath_items.len == 0) { + try dep_subpath.ensureTotalCapacity(dependency.name.len()); + dep_subpath.appendSliceAssumeCapacity(dependency.name.slice(string_buf)); + } else { + try dep_subpath.ensureTotalCapacity(subpath_items.len + 1 + dependency.name.len()); + dep_subpath.appendSliceAssumeCapacity(subpath_items); + dep_subpath.appendAssumeCapacity('/'); + dep_subpath.appendSliceAssumeCapacity(dependency.name.slice(string_buf)); + }Based on past review comments.
567-569: Document why workspace dependencies skip nohoistThe code excludes workspace dependencies from nohoist pattern matching without explanation. Please add a comment explaining the rationale—is it because workspaces have different hoisting semantics, or because they cannot be unhoisted?
Also consider whether catalog dependencies should be similarly excluded.
Based on past review comments.
574-590: Silent fallback on circular dependencies may confuse usersWhen a circular dependency is detected (line 585), the code silently falls back to hoisting despite the user's explicit
nohoistconfiguration. This could be confusing—users might not understand why their nohoist pattern was ignored.Consider one of these approaches:
- Warn the user with a log message explaining the circular dependency prevents nohoist
- Fail with a clear error asking the user to remove the nohoist pattern for that dependency
- Document this behavior if the silent fallback is intentional
Example warning approach:
if (res_id == pkg_id) { if (comptime Environment.allow_assert or builder.manager.options.log_level.isVerbose()) { Output.warn("Cannot apply nohoist pattern '{s}' to {s} due to circular dependency", .{ nohoist_pattern, dependency.name.slice(string_buf), }); } break :try_nohoist; }Based on past review comments.
554-595: Consider extracting nohoist logic to reduce complexityThe nohoist filtering logic adds significant nesting and complexity to an already long function (processSubtree). Past review comments suggested extracting this into helper methods to improve readability and maintainability.
Suggested extractions:
- Cycle detection logic (lines 574-590) →
Builder.pkgHasCircularReference(...)- Pattern matching loop (lines 571-593) →
Builder.shouldNoHoist(...)This would reduce nesting depth and make the code more maintainable. See past review comments for detailed refactoring examples.
Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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 (3)
src/install/PackageManager.zig(2 hunks)src/install/lockfile/Package.zig(1 hunks)src/install/lockfile/Tree.zig(8 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.zigsrc/install/PackageManager.zigsrc/install/lockfile/Package.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)
Files:
src/install/lockfile/Tree.zigsrc/install/PackageManager.zigsrc/install/lockfile/Package.zig
🧠 Learnings (25)
📓 Common learnings
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/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-09-02T19:17:26.376Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-11-12T07:15:00.734Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 24631
File: src/install/resolution.zig:375-376
Timestamp: 2025-11-12T07:15:00.734Z
Learning: In src/install/resolution.zig, the eql() function intentionally returns false for unknown/unhandled tags in the else branch, even though order() returns .eq for matching unknown tags. This semantic difference is by design.
Applied to files:
src/install/lockfile/Tree.zig
📚 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: Be rigorous and test edge cases and unexpected inputs
Applied to files:
src/install/lockfile/Tree.zig
📚 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/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-01T22:13:08.081Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/codegen/bindgenv2/internal/base.ts:120-125
Timestamp: 2025-10-01T22:13:08.081Z
Learning: Iterator.some() and other Iterator helper methods (map, filter, etc.) are supported in Bun and modern Node.js (22+) runtimes and can be safely used on string iterators and other iterator objects.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-08T13:56:00.875Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.
Applied to files:
src/install/lockfile/Tree.zig
📚 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:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-08T03:11:45.286Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-10-08T03:11:45.286Z
Learning: Applies to src/**/*.zig : When adding debug logs in Zig, create a scoped logger and log via Bun APIs: `const log = bun.Output.scoped(.${SCOPE}, .hidden);` then `log("...", .{})`
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/install/lockfile/Tree.zig
📚 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:
src/install/lockfile/Tree.zig
📚 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/src/symbols.txt : Add new V8 API mangled symbols (without leading underscore) to src/symbols.txt
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-09-02T18:27:23.279Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/collections/multi_array_list.zig:24-24
Timestamp: 2025-09-02T18:27:23.279Z
Learning: The `#allocator` syntax in bun's custom Zig implementation is valid and does not require quoting with @"#allocator". Private fields using the `#` prefix work correctly throughout the codebase without special quoting syntax.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-09-02T18:29:58.304Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/memory.zig:37-43
Timestamp: 2025-09-02T18:29:58.304Z
Learning: In bun's `src/memory.zig`, the `initDefault` function intentionally calls `T.init()` without parameter validation to cause compile-time errors when `init` requires arguments or returns an error union. This design forces developers to add explicit `initDefault` methods for complex types rather than silently falling back to `{}`. The `{}` fallback is only intended for types that have no `init` function at all.
Applied to files:
src/install/lockfile/Package.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/Package.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-09-02T17:07:29.377Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators.zig:873-879
Timestamp: 2025-09-02T17:07:29.377Z
Learning: In Zig's bun codebase, in functions like `unpackNullable` that return `?Allocator`, anonymous method calls like `.get()` are preferred over fully qualified calls like `Nullable(Allocator).get(allocator)` because the compiler can infer the correct type from the return type context. This follows Zig's anonymous literal syntax conventions.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-09-02T17:41:07.869Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/memory.zig:60-76
Timestamp: 2025-09-02T17:41:07.869Z
Learning: In bun's memory utilities, when handling const pointers in deinit operations, prefer compile-time errors over silent skipping to avoid hard-to-find memory leaks. Users expect explicit failures rather than silent omissions in memory management.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-11-11T22:55:04.047Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24571
File: src/css/selectors/parser.zig:908-916
Timestamp: 2025-11-11T22:55:04.047Z
Learning: In oven-sh/bun, CSS serialization uses an arena allocator. In src/css/selectors/parser.zig, functions like PseudoClass.toCss and PseudoElement.toCss intentionally do not call deinit on std.Io.Writer.Allocating, scratch buffers, or css.Printer because dest.allocator is an arena and these temporaries are reclaimed when the CSS pass completes. Only debug-only paths (e.g., DeclarationBlock.DebugFmt in src/css/declaration.zig) may explicitly deinit.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-10-14T04:04:47.894Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23636
File: src/shell/ParsedShellScript.zig:15-0
Timestamp: 2025-10-14T04:04:47.894Z
Learning: In src/shell/ParsedShellScript.zig, the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization (via `#computeEstimatedSizeForGC()`) and not updated afterward, even though fields like `args`, `export_env`, and `cwd` may be mutated by methods like `setCwd()`, `setEnv()`, and `take()`. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/install/PackageManager/PackageManagerOptions.zig:187-193
Timestamp: 2025-10-16T17:32:03.074Z
Learning: In Bun's codebase (particularly in files like src/install/PackageManager/PackageManagerOptions.zig), mixing bun.EnvVar.*.get() and bun.EnvVar.*.platformGet() for environment variable lookups is intentional and safe. The code is protected by compile-time platform checks (Environment.isWindows, etc.), and compilation will fail if the wrong function is used on the wrong platform. This pattern should not be flagged as a consistency issue.
Applied to files:
src/install/lockfile/Package.zig
🔇 Additional comments (1)
src/install/PackageManager.zig (1)
1-2: LGTM – Singleton documentation and field initializationThe documentation clearly communicates the lifetime semantics, and the field initialization follows the agreed-upon approach from previous review discussions.
Also applies to: 140-140
| var nohoist_patterns: bun.collections.ArrayListDefault([]const u8) = try .initCapacity(nohoist_arr.items.len); | ||
|
|
||
| for (nohoist_arr.items.slice()) |nohoist_item| { | ||
| if (!nohoist_item.isString()) { | ||
| try log.addError(source, nohoist_item.loc, "Expected a string pattern"); | ||
| return error.InvalidPackageJSON; | ||
| } | ||
|
|
||
| nohoist_patterns.appendAssumeCapacity(try nohoist_item.data.e_string.stringCloned(bun.default_allocator)); | ||
| } | ||
|
|
||
| pm.nohoist_patterns = nohoist_patterns; | ||
| } |
There was a problem hiding this comment.
Potential memory leak in error path
If parsing succeeds for some patterns but fails on a later pattern (line 1594-1596), the successfully cloned strings in nohoist_patterns are leaked because the ArrayList is never assigned to pm.nohoist_patterns and never deinitialized.
Consider deferring cleanup or building directly into pm.nohoist_patterns:
- var nohoist_patterns: bun.collections.ArrayListDefault([]const u8) = try .initCapacity(nohoist_arr.items.len);
+ try pm.nohoist_patterns.ensureTotalCapacity(nohoist_arr.items.len);
+ const original_len = pm.nohoist_patterns.items().len;
+ errdefer {
+ for (pm.nohoist_patterns.items()[original_len..]) |pattern| {
+ bun.default_allocator.free(pattern);
+ }
+ pm.nohoist_patterns.shrinkRetainingCapacity(original_len);
+ }
for (nohoist_arr.items.slice()) |nohoist_item| {
if (!nohoist_item.isString()) {
try log.addError(source, nohoist_item.loc, "Expected a string pattern");
return error.InvalidPackageJSON;
}
- nohoist_patterns.appendAssumeCapacity(try nohoist_item.data.e_string.stringCloned(bun.default_allocator));
+ pm.nohoist_patterns.appendAssumeCapacity(try nohoist_item.data.e_string.stringCloned(bun.default_allocator));
}
-
- pm.nohoist_patterns = nohoist_patterns;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/install/lockfile/Package.zig around lines 1590-1602, the code builds a
local ArrayList of cloned nohoist strings but on an early error path the list
and its allocated strings can leak because it is not deinitialized; to fix this
either (a) defer a cleanup that deinitializes/free the local nohoist_patterns on
any error before returning, or (b) avoid the temporary by initializing
pm.nohoist_patterns first and append directly into it so assignment is
unnecessary, ensuring any failure deinitializes whatever was initialized;
implement one of these approaches so all allocated memory is freed on error and
only set pm.nohoist_patterns after successful completion.
| dependency_id: DependencyID, | ||
| hoist_root_id: Tree.Id, | ||
| comptime method: BuilderMethod, | ||
| subpath: if (method == .filter) bun.collections.ArrayListDefault(u8) else void, |
There was a problem hiding this comment.
We repeat the expression if (method == .filter) bun.collections.ArrayListDefault(u8) else void in multiple places, so I would recommend defining a type alias.
Also, I think we should import ArrayListDefault since we refer to it a lot.
|
|
||
| try dep_subpath.ensureTotalCapacity(subpath.items().len + @intFromBool(subpath.items().len != 0) + dependency.name.len()); | ||
| dep_subpath.appendSliceAssumeCapacity(subpath.items()); | ||
| if (subpath.items().len != 0) { |
There was a problem hiding this comment.
| if (subpath.items().len != 0) { | |
| if (!subpath.isEmpty()) { |
etc. for the other instances of items().len != 0 or items().len == 0
Co-authored-by: taylor.fish <contact@taylor.fish>
Co-authored-by: taylor.fish <contact@taylor.fish>
Co-authored-by: taylor.fish <contact@taylor.fish>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/install/lockfile/Package.zig (2)
1590-1602: Critical: Error path still leaks cloned pattern strings.This issue was previously flagged but remains unresolved. If string validation fails at line 1594 after some patterns have been successfully cloned via
stringCloned()at line 1598, those heap-allocated strings leak because:
- The temporary
nohoist_patternsArrayList is never deinitialized on error- The cloned strings are never freed
pm.nohoist_patternshasn't been assigned yetApply the approach suggested in the previous review to append directly into
pm.nohoist_patternswith proper errdefer cleanup:- var nohoist_patterns: bun.collections.ArrayListDefault([]const u8) = try .initCapacity(nohoist_arr.items.len); + try pm.nohoist_patterns.ensureTotalCapacity(nohoist_arr.items.len); + const original_len = pm.nohoist_patterns.items.len; + errdefer { + for (pm.nohoist_patterns.items[original_len..]) |pattern| { + bun.default_allocator.free(pattern); + } + pm.nohoist_patterns.shrinkRetainingCapacity(original_len); + } for (nohoist_arr.items.slice()) |*nohoist_item| { if (!nohoist_item.isString()) { try log.addError(source, nohoist_item.loc, "Expected a string pattern"); return error.InvalidPackageJSON; } - nohoist_patterns.appendAssumeCapacity(try nohoist_item.data.e_string.stringCloned(bun.default_allocator)); + pm.nohoist_patterns.appendAssumeCapacity(try nohoist_item.data.e_string.stringCloned(bun.default_allocator)); } - - pm.nohoist_patterns = nohoist_patterns;
1592-1592: Use pointer in loop iteration.As suggested by taylordotfish in a previous review, prefer
|*nohoist_item|to avoid copying theExprvalue on each iteration.Apply this diff:
- for (nohoist_arr.items.slice()) |nohoist_item| { + for (nohoist_arr.items.slice()) |*nohoist_item| {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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 (2)
src/install/lockfile.zig(1 hunks)src/install/lockfile/Package.zig(1 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.zigsrc/install/lockfile/Package.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)
Files:
src/install/lockfile.zigsrc/install/lockfile/Package.zig
🧠 Learnings (13)
📓 Common learnings
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/js/third_party/**/*.{js,ts} : Place third-party npm package tests under test/js/third_party/
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: Follow existing patterns in similar V8 classes, add comprehensive Node.js parity tests, update all symbol files, and document any special behavior
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
📚 Learning: 2025-09-02T17:41:07.869Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/memory.zig:60-76
Timestamp: 2025-09-02T17:41:07.869Z
Learning: In bun's memory utilities, when handling const pointers in deinit operations, prefer compile-time errors over silent skipping to avoid hard-to-find memory leaks. Users expect explicit failures rather than silent omissions in memory management.
Applied to files:
src/install/lockfile.zigsrc/install/lockfile/Package.zig
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-09-02T18:29:58.304Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/memory.zig:37-43
Timestamp: 2025-09-02T18:29:58.304Z
Learning: In bun's `src/memory.zig`, the `initDefault` function intentionally calls `T.init()` without parameter validation to cause compile-time errors when `init` requires arguments or returns an error union. This design forces developers to add explicit `initDefault` methods for complex types rather than silently falling back to `{}`. The `{}` fallback is only intended for types that have no `init` function at all.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-09-02T18:27:23.279Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/collections/multi_array_list.zig:24-24
Timestamp: 2025-09-02T18:27:23.279Z
Learning: The `#allocator` syntax in bun's custom Zig implementation is valid and does not require quoting with @"#allocator". Private fields using the `#` prefix work correctly throughout the codebase without special quoting syntax.
Applied to files:
src/install/lockfile/Package.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/Package.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-09-02T17:07:29.377Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators.zig:873-879
Timestamp: 2025-09-02T17:07:29.377Z
Learning: In Zig's bun codebase, in functions like `unpackNullable` that return `?Allocator`, anonymous method calls like `.get()` are preferred over fully qualified calls like `Nullable(Allocator).get(allocator)` because the compiler can infer the correct type from the return type context. This follows Zig's anonymous literal syntax conventions.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-11-11T22:55:04.047Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24571
File: src/css/selectors/parser.zig:908-916
Timestamp: 2025-11-11T22:55:04.047Z
Learning: In oven-sh/bun, CSS serialization uses an arena allocator. In src/css/selectors/parser.zig, functions like PseudoClass.toCss and PseudoElement.toCss intentionally do not call deinit on std.Io.Writer.Allocating, scratch buffers, or css.Printer because dest.allocator is an arena and these temporaries are reclaimed when the CSS pass completes. Only debug-only paths (e.g., DeclarationBlock.DebugFmt in src/css/declaration.zig) may explicitly deinit.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-10-14T04:04:47.894Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23636
File: src/shell/ParsedShellScript.zig:15-0
Timestamp: 2025-10-14T04:04:47.894Z
Learning: In src/shell/ParsedShellScript.zig, the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization (via `#computeEstimatedSizeForGC()`) and not updated afterward, even though fields like `args`, `export_env`, and `cwd` may be mutated by methods like `setCwd()`, `setEnv()`, and `take()`. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/install/PackageManager/PackageManagerOptions.zig:187-193
Timestamp: 2025-10-16T17:32:03.074Z
Learning: In Bun's codebase (particularly in files like src/install/PackageManager/PackageManagerOptions.zig), mixing bun.EnvVar.*.get() and bun.EnvVar.*.platformGet() for environment variable lookups is intentional and safe. The code is protected by compile-time platform checks (Environment.isWindows, etc.), and compilation will fail if the wrong function is used on the wrong platform. This pattern should not be flagged as a consistency issue.
Applied to files:
src/install/lockfile/Package.zig
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/install/lockfile/Package.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 : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/install/lockfile/Package.zig
| var subpath = item.subpath; | ||
|
|
||
| defer bun.memory.deinit(&subpath); |
There was a problem hiding this comment.
Make the deferred deinitialization conditional on method.
The defer bun.memory.deinit(&subpath) executes unconditionally, but when method != .filter, the subpath appears to be empty (based on line 954 passing {}). While Zig's comptime optimizations may handle this, it's clearer and safer to conditionally defer the deinitialization only when there's actual data to free.
Apply this diff to guard the defer:
while (builder.queue.readItem()) |item| {
var subpath = item.subpath;
-
- defer bun.memory.deinit(&subpath);
-
+ if (comptime method == .filter) {
+ defer bun.memory.deinit(&subpath);
+ }
try builder.list.items(.tree)[item.tree_id].processSubtree(
item.dependency_id,
item.hoist_root_id,
method,
subpath,
&builder,
);
}This mirrors the conditional pattern used on line 954 and aligns with the learning that memory utilities should be explicit about when cleanup is needed. Based on learnings
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/install/lockfile.zig around lines 960 to 962, the defer
bun.memory.deinit(&subpath) runs unconditionally even though subpath is only
populated when method == .filter; change the code to conditionally defer
deinitialization only when method == .filter (i.e., wrap the defer call in the
same condition used when subpath is created), so that bun.memory.deinit is only
registered for cleanup when there is actual memory to free.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/install/lockfile/Tree.zig (2)
562-602: Verify silent fallback to hoisting on circular dependencies matches expected behavior.When a circular dependency is detected (line 592), the code silently falls back to hoisting via
break :try_nohoist. This behavior may be unexpected for users who explicitly configurednohoistpatterns. Consider whether an error should be reported instead, or at minimum, log a warning so users understand why their nohoist pattern was ignored.As per existing review comment from markovejnovic: "If I were a user and I specifically told my build system to not hoist my damn dependency, I wouldn't want there to ever be a case in which the package manager decided to hoist it, especially silently."
574-576: Verify workspace dependency exclusion from nohoist is intentional.Workspace dependencies are explicitly excluded from nohoist processing. Is this because they cannot be hoisted, or is there another reason? If workspace dependencies cannot be hoisted, consider also checking for catalog dependencies which may have similar constraints.
As per existing review comment from markovejnovic regarding coupling and the suggestion to add a
supportsHoisting()method to theTagenum.src/install/lockfile.zig (1)
960-961: Conditionally defer deinitialization based on method.The
defer bun.memory.deinit(&subpath)executes unconditionally, but whenmethod != .filter,subpathisvoid(as returned bySubpath(method)). While Zig's comptime evaluation may optimize this away, it's clearer and safer to guard the defer with the same condition used during initialization.Apply this diff to conditionally defer:
while (builder.queue.readItem()) |item| { var subpath = item.subpath; - defer bun.memory.deinit(&subpath); + if (comptime method == .filter) { + defer bun.memory.deinit(&subpath); + } try builder.list.items(.tree)[item.tree_id].processSubtree(Based on learnings about explicit memory management and past review comments from coderabbitai[bot] and taylordotfish.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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 (2)
src/install/lockfile.zig(1 hunks)src/install/lockfile/Tree.zig(8 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.zigsrc/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)
Files:
src/install/lockfile.zigsrc/install/lockfile/Tree.zig
🧠 Learnings (20)
📚 Learning: 2025-09-02T17:41:07.869Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/memory.zig:60-76
Timestamp: 2025-09-02T17:41:07.869Z
Learning: In bun's memory utilities, when handling const pointers in deinit operations, prefer compile-time errors over silent skipping to avoid hard-to-find memory leaks. Users expect explicit failures rather than silent omissions in memory management.
Applied to files:
src/install/lockfile.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.zig
📚 Learning: 2025-09-02T19:17:26.376Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.
Applied to files:
src/install/lockfile.zigsrc/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/install/lockfile.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In finalize() for objects holding JS references, release them using .deref() before destroy
Applied to files:
src/install/lockfile.zig
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/install/lockfile.zig
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/install/lockfile.zigsrc/install/lockfile/Tree.zig
📚 Learning: 2025-09-02T18:29:58.304Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/memory.zig:37-43
Timestamp: 2025-09-02T18:29:58.304Z
Learning: In bun's `src/memory.zig`, the `initDefault` function intentionally calls `T.init()` without parameter validation to cause compile-time errors when `init` requires arguments or returns an error union. This design forces developers to add explicit `initDefault` methods for complex types rather than silently falling back to `{}`. The `{}` fallback is only intended for types that have no `init` function at all.
Applied to files:
src/install/lockfile.zig
📚 Learning: 2025-11-12T07:15:00.734Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 24631
File: src/install/resolution.zig:375-376
Timestamp: 2025-11-12T07:15:00.734Z
Learning: In src/install/resolution.zig, the eql() function intentionally returns false for unknown/unhandled tags in the else branch, even though order() returns .eq for matching unknown tags. This semantic difference is by design.
Applied to files:
src/install/lockfile/Tree.zig
📚 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: Be rigorous and test edge cases and unexpected inputs
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-08T13:56:00.875Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
Applied to files:
src/install/lockfile/Tree.zig
📚 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/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
src/install/lockfile/Tree.zig
📚 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:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-01T22:13:08.081Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/codegen/bindgenv2/internal/base.ts:120-125
Timestamp: 2025-10-01T22:13:08.081Z
Learning: Iterator.some() and other Iterator helper methods (map, filter, etc.) are supported in Bun and modern Node.js (22+) runtimes and can be safely used on string iterators and other iterator objects.
Applied to files:
src/install/lockfile/Tree.zig
📚 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:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/bun/**/*.{ts,js} : Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-19T02:52:37.412Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/tsconfig.json:1-15
Timestamp: 2025-10-19T02:52:37.412Z
Learning: In the Bun repository, packages under packages/ (e.g., bun-otel) can follow a TypeScript-first pattern where package.json exports point directly to .ts files (not compiled .js files). Bun natively runs TypeScript, so consumers import .ts sources directly and receive full type information without needing compiled .d.ts declaration files. For such packages, adding "declaration": true or "outDir" in tsconfig.json is unnecessary and would break the export structure.
<!-- [remove_learning]
ceedde95-980e-4898-a2c6-40ff73913664
Applied to files:
src/install/lockfile/Tree.zig
📚 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:
src/install/lockfile/Tree.zig
📚 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: For Node.js compatibility bugs, validate expected behavior in Node.js first, then confirm failure on Bun canary
Applied to files:
src/install/lockfile/Tree.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (9)
src/install/lockfile/Tree.zig (7)
252-252: LGTM!The type alias for
ArrayListDefaultimproves code readability and reduces repetition throughout the file.
259-270: LGTM!Moving
FillItemandTreeFillerintoBuilderas nested declarations improves encapsulation and makes the relationship between these types clearer.
463-468: LGTM!The
Subpath(method)helper provides clean compile-time conditional typing, returningArrayListDefault(u8)for filter operations andvoidotherwise.
567-572: LGTM!The subpath building logic correctly constructs the package path by appending the parent subpath, a separator (only if parent is non-empty), and the dependency name. The capacity pre-allocation avoids repeated allocations.
578-600: LGTM!The glob pattern matching correctly identifies packages that should not be hoisted, and the circular dependency check prevents infinite loops in the dependency graph. The logic properly walks up the tree to verify no ancestor has the same package resolution.
683-683: LGTM!The conditional assignment of
dep_subpathtoFillItem.subpathcorrectly ensures that subpath data is only transferred when using the filter method, avoiding unnecessary memory operations for the resolvable method.Also applies to: 707-707
843-843: LGTM!The
globalias improves readability and the usage at line 445 (and elsewhere) correctly invokes the glob matching logic.Also applies to: 445-445
src/install/lockfile.zig (2)
954-954: LGTM!The conditional initialization of an empty subpath accumulator for the filter method is correct. For the resolvable method,
{}(void) is passed, which matches theSubpath(method)return type.
967-967: LGTM!The
subpathparameter is correctly passed toprocessSubtree, maintaining the per-item subpath context throughout the breadth-first traversal.
| for (builder.sort_buf.items) |dep_id| { | ||
| const dependency = builder.dependencies[dep_id]; | ||
| const pkg_id = builder.resolutions[dep_id]; | ||
|
|
||
| var dep_subpath: ArrayListDefault(u8) = .init(); |
There was a problem hiding this comment.
Memory leak: dep_subpath is never deinitialized.
The dep_subpath ArrayList is initialized on line 523 but is never cleaned up in any code path. This will leak memory for every dependency processed during filtering.
Apply this diff to add proper cleanup:
for (builder.sort_buf.items) |dep_id| {
const dependency = builder.dependencies[dep_id];
const pkg_id = builder.resolutions[dep_id];
var dep_subpath: ArrayListDefault(u8) = .init();
+ defer bun.memory.deinit(&dep_subpath);
// filter out disabled dependencies🤖 Prompt for AI Agents
In src/install/lockfile/Tree.zig around lines 519-523, the ArrayList dep_subpath
is initialized for each dependency but never deinitialized, leaking memory on
every iteration; ensure dep_subpath is always cleaned up by calling its deinit
in every control path — simplest fix: immediately arrange for cleanup after
initialization (e.g., use a defer-like pattern so dep_subpath.deinit() runs when
the loop iteration exits) or explicitly call dep_subpath.deinit() before any
continue/return/branch that skips the normal end-of-iteration, so every created
ArrayList is deinitialized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/install/lockfile/Tree.zig (3)
523-523: Critical: Memory leak -dep_subpathis never deinitialized.The
dep_subpathArrayList is initialized on each loop iteration but is never cleaned up. This leaks memory for every dependency processed during filtering. Multiple code paths (continue statements at lines 535, 541, 555, 644) can exit the loop iteration without cleanup.Add cleanup immediately after initialization:
for (builder.sort_buf.items) |dep_id| { const dependency = builder.dependencies[dep_id]; const pkg_id = builder.resolutions[dep_id]; var dep_subpath: ArrayListDefault(u8) = .init(); + defer bun.memory.deinit(&dep_subpath); // filter out disabled dependenciesThis ensures cleanup happens on all code paths (normal iteration end, continue, or early return).
581-597: Consider warning users when nohoist cannot be satisfied due to circular dependencies.When a circular dependency is detected (line 592), the code silently falls back to hoisting. However, users who explicitly specified a nohoist pattern might expect an error or warning when their directive cannot be satisfied, rather than silent fallback behavior.
Consider logging a warning or error message informing users that nohoist was skipped due to circular dependencies, so they can either fix the circular dependency or adjust their nohoist patterns.
Based on past review comments.
463-468: Consider usingbun.StringBuilderinstead ofArrayListDefault(u8)for string building.Since
Subpathis used for building path strings,bun.StringBuilderwould be more semantically appropriate and may offer better ergonomics for string operations.Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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(8 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)
Files:
src/install/lockfile/Tree.zig
🧠 Learnings (19)
📓 Common learnings
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: Follow existing patterns in similar V8 classes, add comprehensive Node.js parity tests, update all symbol files, and document any special behavior
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/js/third_party/**/*.{js,ts} : Place third-party npm package tests under test/js/third_party/
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-09-02T19:17:26.376Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-11-12T07:15:00.734Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 24631
File: src/install/resolution.zig:375-376
Timestamp: 2025-11-12T07:15:00.734Z
Learning: In src/install/resolution.zig, the eql() function intentionally returns false for unknown/unhandled tags in the else branch, even though order() returns .eq for matching unknown tags. This semantic difference is by design.
Applied to files:
src/install/lockfile/Tree.zig
📚 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: Be rigorous and test edge cases and unexpected inputs
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-08T13:56:00.875Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
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-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In finalize() for objects holding JS references, release them using .deref() before destroy
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-01T22:13:08.081Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/codegen/bindgenv2/internal/base.ts:120-125
Timestamp: 2025-10-01T22:13:08.081Z
Learning: Iterator.some() and other Iterator helper methods (map, filter, etc.) are supported in Bun and modern Node.js (22+) runtimes and can be safely used on string iterators and other iterator objects.
Applied to files:
src/install/lockfile/Tree.zig
📚 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/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
src/install/lockfile/Tree.zig
📚 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:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-08T03:11:45.286Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-10-08T03:11:45.286Z
Learning: Applies to src/**/*.zig : When adding debug logs in Zig, create a scoped logger and log via Bun APIs: `const log = bun.Output.scoped(.${SCOPE}, .hidden);` then `log("...", .{})`
Applied to files:
src/install/lockfile/Tree.zig
📚 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/V8*.h : Ensure all public V8 API functions are marked with BUN_EXPORT for symbol visibility
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-18T01:49:31.037Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/SocketConfig.bindv2.ts:58-58
Timestamp: 2025-10-18T01:49:31.037Z
Learning: In Bun's bindgenv2 TypeScript bindings (e.g., src/bun.js/api/bun/socket/SocketConfig.bindv2.ts), the pattern `b.String.loose.nullable.loose` is intentional and not a duplicate. The first `.loose` applies to the String type (loose string conversion), while the second `.loose` applies to the nullable (loose nullable, treating all falsy values as null rather than just null/undefined).
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Applied to files:
src/install/lockfile/Tree.zig
🔇 Additional comments (3)
src/install/lockfile/Tree.zig (3)
683-683: Subpath propagation looks correct.The conditional passing of
subpathtowriteItemusing comptime method checking is appropriate. This ensures subpath is only passed when method is.filter.Note: Assuming
writeItemproperly handles ownership transfer or copying of the ArrayList. However, this doesn't resolve the memory leak at line 523 - cleanup is still required at the allocation site.Also applies to: 707-707
843-845: LGTM: Local aliases improve readability.Adding local aliases for commonly used types (
glob,ArrayListDefault) improves code readability throughout the file.
574-576: The review comment conflates two fundamentally different dependency types and reaches an incorrect conclusion.Workspace and catalog dependencies are not analogous:
Workspace dependencies (
.workspacetag) represent local monorepo packages with relative paths. They should never be hoisted because they're local to the workspace.Catalog dependencies (
.catalogtag) are version indirection references that resolve to actual npm, git, or other package types. The hoisting behavior should depend on the resolved package type, not on the catalog reference itself.Excluding all catalog dependencies from nohoist—as suggested by the review comment—would prevent valid hoisting of packages defined in catalogs, since these ultimately resolve to hoistable npm/git/tarball packages. The current code correctly only excludes workspace dependencies.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/install/lockfile/Tree.zig (2)
525-564: Critical: Memory leak -dep_subpathis never deinitialized.The
dep_subpathArrayList is initialized on line 529 but is never cleaned up. Multiplecontinuestatements (lines 541, 547, 561) and later paths (line 650) exit the loop iteration without deinitializingdep_subpath, leaking memory on every iteration that doesn't reach thewriteItemcalls at lines 689 or 713.Apply this diff to add proper cleanup immediately after initialization:
for (builder.sort_buf.items) |dep_id| { const dependency = builder.dependencies[dep_id]; const pkg_id = builder.resolutions[dep_id]; var dep_subpath: ArrayListDefault(u8) = .init(); + defer dep_subpath.deinit(); // filter out disabled dependencies
568-608: Consider refactoring for improved readability.This nohoist pattern matching logic is deeply nested (4-5 levels) and spans 40 lines. Past review comments suggested extracting the circular dependency check (lines 587-603) into a separate helper method to reduce nesting and improve maintainability.
While the logic appears correct, the complexity makes it harder to reason about edge cases and increases the likelihood of bugs during future modifications.
See the detailed refactoring suggestions in the past review comments by markovejnovic, which demonstrate how to extract
isTreeCircular()and reduce nesting by inverting conditions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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(8 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)
Files:
src/install/lockfile/Tree.zig
🧠 Learnings (18)
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-09-02T19:17:26.376Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-11-12T07:15:00.734Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 24631
File: src/install/resolution.zig:375-376
Timestamp: 2025-11-12T07:15:00.734Z
Learning: In src/install/resolution.zig, the eql() function intentionally returns false for unknown/unhandled tags in the else branch, even though order() returns .eq for matching unknown tags. This semantic difference is by design.
Applied to files:
src/install/lockfile/Tree.zig
📚 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: Be rigorous and test edge cases and unexpected inputs
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-08T13:56:00.875Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
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-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In finalize() for objects holding JS references, release them using .deref() before destroy
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-01T22:13:08.081Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/codegen/bindgenv2/internal/base.ts:120-125
Timestamp: 2025-10-01T22:13:08.081Z
Learning: Iterator.some() and other Iterator helper methods (map, filter, etc.) are supported in Bun and modern Node.js (22+) runtimes and can be safely used on string iterators and other iterator objects.
Applied to files:
src/install/lockfile/Tree.zig
📚 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/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
src/install/lockfile/Tree.zig
📚 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:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-08T03:11:45.286Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-10-08T03:11:45.286Z
Learning: Applies to src/**/*.zig : When adding debug logs in Zig, create a scoped logger and log via Bun APIs: `const log = bun.Output.scoped(.${SCOPE}, .hidden);` then `log("...", .{})`
Applied to files:
src/install/lockfile/Tree.zig
📚 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/V8*.h : Ensure all public V8 API functions are marked with BUN_EXPORT for symbol visibility
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-18T01:49:31.037Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/SocketConfig.bindv2.ts:58-58
Timestamp: 2025-10-18T01:49:31.037Z
Learning: In Bun's bindgenv2 TypeScript bindings (e.g., src/bun.js/api/bun/socket/SocketConfig.bindv2.ts), the pattern `b.String.loose.nullable.loose` is intentional and not a duplicate. The first `.loose` applies to the String type (loose string conversion), while the second `.loose` applies to the nullable (loose nullable, treating all falsy values as null rather than just null/undefined).
Applied to files:
src/install/lockfile/Tree.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Applied to files:
src/install/lockfile/Tree.zig
🔇 Additional comments (5)
src/install/lockfile/Tree.zig (5)
252-252: LGTM! Good refactoring with type alias.The introduction of the
ArrayListDefaultmodule-level alias improves readability and reduces verbosity throughout the file.Also applies to: 851-851
259-276: LGTM! Improved encapsulation and documentation.Moving
FillItemandTreeFillerinsideBuilderas nested public types improves code organization. The doc comment forsubpathclearly explains its purpose with helpful examples.
469-474: LGTM! Clean comptime type abstraction.The
Subpathhelper function effectively uses Zig's comptime features to conditionally define the subpath type based on the builder method.
481-481: LGTM! Signature updated to support subpath propagation.The addition of the
subpathparameter is necessary for nohoist pattern matching and integrates cleanly with theSubpathtype helper.
849-849: LGTM! Convenient alias.The
globalias improves readability by reducing verbosity throughout the file.
| .tree_id = replace.id, | ||
| .dependency_id = dep_id, | ||
| .hoist_root_id = hoist_root_id, | ||
| .subpath = if (comptime method == .filter) dep_subpath else {}, |
There was a problem hiding this comment.
Ensure transferred ownership is properly managed.
When dep_subpath is assigned to .subpath at these lines, ownership of the internal buffer is transferred. However, the local dep_subpath variable still holds a copy of the struct with pointers to that buffer.
After fixing the memory leak identified earlier by adding a defer dep_subpath.deinit();, you must also reset dep_subpath immediately after these assignments to prevent double-free:
.subpath = dep_subpath,
dep_subpath = .init(); // Reset to prevent defer from freeing transferred bufferAlternatively, use a boolean flag to track whether ownership was transferred and conditionally deinit in the defer.
Also applies to: 713-713
🤖 Prompt for AI Agents
In src/install/lockfile/Tree.zig around lines 689 and 713, assigning dep_subpath
into the .subpath field transfers ownership of the internal buffer but leaves
the local dep_subpath holding pointers, which will cause a double-free when its
defer runs; after each assignment that transfers ownership, reset dep_subpath to
an empty/initial state (or flip a boolean “transferred” flag) so the deferred
deinit does not free the transferred buffer, or alternatively track transfer
with a flag and conditionally call deinit only when ownership was not moved.
What does this PR do?
Adds support for
"workspaces.nohoist"inbun install.{ "name": "nohoist-settings", "workspaces": { "nohoist": ["pkg1/react", "pkg2/**", "**/react"], "packages": ["pkg1", "pkg2"] }, "dependencies": { "react": "^18" } }closes #6850
"nohoist"The
"nohoist"field prevents a package from hoisting if any of the specified patterns match it's package path. The patterns support glob syntax. Example:package.json
{ "name": "nohoist-react", "workspaces": { "packages": [ "pkg1" ], "nohoist": [ "pkg1/react" ] } }pkg1/package.json:
{ "name": "pkg1", "dependencies": { "react": "18.3.1" } }This package.json will prevent react@18.3.1 from hoisting out of pkg1. The transitive dependencies of react are still able to hoist to the root. The result of an install would be:
If you wanted to prevent react and all it's dependencies from hoisting, "pkg1/react/**" could be added to nohoist:
"nohoist": [ + "pkg1/react/**", "pkg1/react" }An install would create:
closes ENG-21480
How did you verify your code works?
TODO:
webpack-cliand"nohoist": ["**"]hangs (it was circular dependencies)