Skip to content

fix: don't create empty node_modules when no dependencies exist #24573

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

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

fix: don't create empty node_modules when no dependencies exist #24573

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/install/hoisted_install.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ pub fn installHoistedPackages(
}
}

// If there are no dependencies to install and the original lockfile had no dependencies either, don't create node_modules
// We still create node_modules if the original lockfile has packages (e.g., with --production filtering devDeps)
if (this.lockfile.buffers.hoisted_dependencies.items.len == 0 and original_tree_dep_ids.items.len == 0) {
return PackageInstall.Summary{};
}

// If there was already a valid lockfile and so we did not resolve, i.e. there was zero network activity
// the packages could still not be in the cache dir
// this would be a common scenario in a CI environment
Expand Down
6 changes: 6 additions & 0 deletions src/install/isolated_install.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,12 @@ pub fn installIsolatedPackages(
};
};

// If there are no packages to install and no lockfile packages, don't create node_modules
// We still create node_modules if lockfile has packages (e.g., with --production filtering devDeps)
if (store.entries.len == 0 and lockfile.packages.len == 0) {
return PackageInstall.Summary{};
}

// setup node_modules/.bun
const is_new_bun_modules = is_new_bun_modules: {
const node_modules_path = bun.OSPathLiteral("node_modules");
Expand Down
60 changes: 60 additions & 0 deletions test/regression/issue/5392.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, tempDir } from "harness";
import * as fs from "node:fs";

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: {},
Comment on lines +11 to +12
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.

}),
});

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]);
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.


// 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]);
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


// node_modules should exist
const nodeModulesPath = `${dir}/node_modules`;
const nodeModulesExists = fs.existsSync(nodeModulesPath);

expect(nodeModulesExists).toBe(true);
expect(exitCode).toBe(0);
});
Comment on lines +5 to +60
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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:

  1. Package.json with empty dependencies: {} object (without devDependencies/peerDependencies)
  2. Package.json with only peerDependencies populated
  3. Using --production flag 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.