fix: don't create empty node_modules when no dependencies exist #24573
Conversation
|
Updated 10:10 PM PT - Nov 10th, 2025
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 24573That installs a local version of the PR into your bun-24573 --bun |
WalkthroughAdds early-return guards in installHoistedPackages and installIsolatedPackages to exit before creating node_modules and performing installation work when there are no dependencies to install. Adds regression tests verifying bun install does not create node_modules for empty dependency sets and does for declared dependencies. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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: 1
📜 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/hoisted_install.zig(1 hunks)src/install/isolated_install.zig(1 hunks)test/regression/issue/5392.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/hoisted_install.zigsrc/install/isolated_install.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/hoisted_install.zigsrc/install/isolated_install.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/regression/issue/5392.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/regression/issue/5392.test.ts
🧠 Learnings (13)
📓 Common learnings
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.
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/)
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/node/test/** : Vendored Node.js tests under test/js/node/test/ are exceptions and do not follow Bun’s test style
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
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
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)
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
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/
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/bundler/**/* : Place bundler/transpiler/CSS/bun build tests under test/bundler/
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/
📚 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/regression/issue/5392.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/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/regression/issue/5392.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} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/regression/issue/5392.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/regression/issue/5392.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : Place CLI command tests (e.g., bun install, bun init) under test/cli/
Applied to files:
test/regression/issue/5392.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/node/test/** : Vendored Node.js tests under test/js/node/test/ are exceptions and do not follow Bun’s test style
Applied to files:
test/regression/issue/5392.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/regression/issue/5392.test.ts
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/regression/issue/5392.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/regression/issue/5392.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/regression/issue/5392.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/regression/issue/5392.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: For Node.js compatibility bugs, validate expected behavior in Node.js first, then confirm failure on Bun canary
Applied to files:
test/regression/issue/5392.test.ts
🧬 Code graph analysis (1)
test/regression/issue/5392.test.ts (1)
test/harness.ts (2)
tempDir(277-284)bunExe(102-105)
🔇 Additional comments (3)
src/install/isolated_install.zig (1)
617-620: Early return is correctly placed to prevent node_modules creation.The guard is optimally positioned after store creation but before expensive node_modules filesystem setup. The store creation work (lines 15-615) is necessary to determine whether there are packages to install, making this the right place to exit early.
test/regression/issue/5392.test.ts (2)
5-32: Test correctly validates the no-dependency scenario.The test properly reproduces issue #5392 by creating a package.json with empty dependency objects and verifying that
node_modulesis not created. The use oftempDirwithusingfor automatic cleanup, andbunExe()/bunEnvfrom the harness follows Bun testing patterns.Based on learnings.
34-60: Test provides essential regression coverage for normal install behavior.This test ensures the fix doesn't break the standard case where dependencies exist. The test structure mirrors the first test for consistency, and verifying
node_modulescreation with a real dependency validates that normal installation behavior remains intact.
Fixes #5392 When running `bun install` on a package.json with no dependencies, an empty `node_modules` folder was being created. This change adds checks in both hoisted and isolated install strategies to skip node_modules creation when there are no packages to install. The fix preserves node_modules creation when using filters like `--production` that may result in zero packages after filtering but had packages in the original lockfile. Changes: - Check both filtered and original dependency counts in hoisted install - Check both store entries and lockfile packages in isolated install - Return early with empty Summary if truly no dependencies - Add regression test that verifies no node_modules created when empty 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
robobun
force-pushed
the
claude/fix-empty-node-modules-5392
branch
from
3660b74 to
a60131e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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/hoisted_install.zig(1 hunks)src/install/isolated_install.zig(1 hunks)test/regression/issue/5392.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/hoisted_install.zigsrc/install/isolated_install.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/hoisted_install.zigsrc/install/isolated_install.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/regression/issue/5392.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/regression/issue/5392.test.ts
🧠 Learnings (13)
📓 Common learnings
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.
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/)
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/node/test/** : Vendored Node.js tests under test/js/node/test/ are exceptions and do not follow Bun’s test style
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
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/
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
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/bundler/**/* : Place bundler/transpiler/CSS/bun build tests under test/bundler/
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
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.
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/
📚 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/regression/issue/5392.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/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/regression/issue/5392.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} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/regression/issue/5392.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/regression/issue/5392.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : Place CLI command tests (e.g., bun install, bun init) under test/cli/
Applied to files:
test/regression/issue/5392.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/node/test/** : Vendored Node.js tests under test/js/node/test/ are exceptions and do not follow Bun’s test style
Applied to files:
test/regression/issue/5392.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/regression/issue/5392.test.ts
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/regression/issue/5392.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/regression/issue/5392.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/regression/issue/5392.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/regression/issue/5392.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: For Node.js compatibility bugs, validate expected behavior in Node.js first, then confirm failure on Bun canary
Applied to files:
test/regression/issue/5392.test.ts
🧬 Code graph analysis (1)
test/regression/issue/5392.test.ts (1)
test/harness.ts (2)
tempDir(277-284)bunExe(102-105)
🔇 Additional comments (2)
src/install/isolated_install.zig (1)
617-621: LGTM! Early-return guard correctly prevents empty node_modules creation.The logic is sound: skipping node_modules creation when there are no entries to install AND no lockfile packages. The dual condition correctly handles the filtering scenario (e.g.,
--production) where the lockfile might have packages but the filtered store is empty.src/install/hoisted_install.zig (1)
46-50: LGTM! Early-return guard correctly prevents empty node_modules creation.The logic correctly checks both the filtered dependency count and the original lockfile state, ensuring node_modules is still created when filtering (e.g.,
--production) removes dependencies but the original lockfile had packages.Note: A past review comment suggested moving this check before progress node initialization (line 28) to avoid allocating unused resources. While this would be a minor optimization, the current placement is acceptable since the check must occur after
lockfile.filter()to obtain the filtered count.
| test("bun install should not create node_modules when there are no dependencies - issue #5392", async () => { | ||
| using dir = tempDir("issue-5392", { | ||
| "package.json": JSON.stringify({ | ||
| name: "bun-install-test", | ||
| module: "index.ts", | ||
| type: "module", | ||
| devDependencies: {}, | ||
| peerDependencies: {}, | ||
| }), | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "install"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stderr: "pipe", | ||
| stdout: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| // node_modules should not exist | ||
| const nodeModulesPath = `${dir}/node_modules`; | ||
| const nodeModulesExists = fs.existsSync(nodeModulesPath); | ||
|
|
||
| expect(nodeModulesExists).toBe(false); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("bun install should create node_modules when there are dependencies", async () => { | ||
| using dir = tempDir("issue-5392-with-deps", { | ||
| "package.json": JSON.stringify({ | ||
| name: "bun-install-test-with-deps", | ||
| dependencies: { | ||
| "is-odd": "^3.0.1", | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "install"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stderr: "pipe", | ||
| stdout: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| // node_modules should exist | ||
| const nodeModulesPath = `${dir}/node_modules`; | ||
| const nodeModulesExists = fs.existsSync(nodeModulesPath); | ||
|
|
||
| expect(nodeModulesExists).toBe(true); | ||
| expect(exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding edge case tests for comprehensive coverage.
The current tests cover the primary scenarios well. To enhance robustness, consider adding tests for:
- Package.json with empty
dependencies: {}object (without devDependencies/peerDependencies) - Package.json with only
peerDependenciespopulated - Using
--productionflag with devDependencies (mentioned in code comments)
Example additional test case:
test("bun install should not create node_modules with only empty dependencies object", async () => {
using dir = tempDir("issue-5392-empty-deps", {
"package.json": JSON.stringify({
name: "test",
dependencies: {},
}),
});
await using proc = Bun.spawn({
cmd: [bunExe(), "install"],
env: bunEnv,
cwd: String(dir),
stderr: "pipe",
stdout: "pipe",
});
await proc.exited;
expect(fs.existsSync(`${dir}/node_modules`)).toBe(false);
});🤖 Prompt for AI Agents
In test/regression/issue/5392.test.ts around lines 5 to 60, add three edge-case
tests: (1) package.json with an explicit empty dependencies: {} should not
create node_modules; (2) package.json with only peerDependencies populated
should not create node_modules (peer deps are not installed by default); and (3)
package.json with devDependencies populated and invoking Bun.spawn with the
"--production" flag should not create node_modules for dev deps. For each test
follow the existing pattern: create a tempDir with the appropriate package.json,
spawn Bun.install (include env, cwd, stdout/stderr pipes), await proc.exited,
assert fs.existsSync(`${dir}/node_modules`) matches expected boolean, and assert
exitCode is 0; keep naming consistent and ensure proper cleanup with using/await
using.
| devDependencies: {}, | ||
| peerDependencies: {}, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unnecessary empty dependency objects.
Explicitly specifying empty peerDependencies is unnecessary. Consider removing lines 11-12 to simplify the test case and focus on the core scenario.
name: "bun-install-test",
module: "index.ts",
type: "module",
devDependencies: {},
- peerDependencies: {},
}),📝 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.
| devDependencies: {}, | |
| peerDependencies: {}, | |
| devDependencies: {}, | |
| }), |
🤖 Prompt for AI Agents
In test/regression/issue/5392.test.ts around lines 11 to 12, remove the
unnecessary empty dependency objects devDependencies: {} and peerDependencies:
{} from the test fixture; delete those two lines so the test focuses on the core
scenario and avoids cluttering the package shape with empty objects.
| stdout: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify stderr and stdout or remove unused variables.
The test captures stdout and stderr but doesn't verify them. Consider adding assertions to ensure no errors occurred, or remove the unused variables to clean up the test.
Example enhancement:
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+ const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
+
+ // Optionally verify no errors in output
+ expect(stderr).not.toContain("error:");📝 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.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); | |
| // Optionally verify no errors in output | |
| expect(stderr).not.toContain("error:"); |
🤖 Prompt for AI Agents
In test/regression/issue/5392.test.ts around line 24, the test collects stdout
and stderr into variables but never asserts on them; either assert expected
output (e.g., expect(stderr).toBeEmpty() or expect(stderr).toMatch(/no error/)
and assert stdout contains the expected text) or remove the unused stdout/stderr
captures and only await proc.exited to avoid unused variables; update the code
to either add explicit assertions that stderr is empty and stdout matches the
expected output for the test scenario or drop those variables and only wait for
process completion.
| stdout: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify stderr and stdout or remove unused variables.
Same as test 1: the test captures stdout and stderr but doesn't verify them. Consider adding assertions or removing unused variables.
🤖 Prompt for AI Agents
In test/regression/issue/5392.test.ts around line 52, the test awaits stdout and
stderr but never asserts their contents; either add explicit assertions for
stdout and/or stderr to validate expected output (e.g., check for specific
strings, emptiness, or absence of errors) or stop collecting them and only await
proc.exited (remove stdout/stderr variables and Promise entries) so there are no
unused variables; update the Promise.all call and test assertions accordingly to
ensure captured streams are verified or not collected.
Summary
bun installcreates an emptynode_modulesfolder when package.json has no dependenciesChanges
hoisted_dependencies.items.len == 0and return earlystore.entries.len == 0and return earlyTest plan
USE_SYSTEM_BUN=1 bun test test/regression/issue/5392.test.tsbun bd test test/regression/issue/5392.test.tsnode_modulescreatednode_modulescreated correctlyFixes #5392
🤖 Generated with Claude Code