Skip to content

bundler: fix ESMConditions capacity with user-provided conditions #30470

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

bundler: fix ESMConditions capacity with user-provided conditions #30470

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
7 changes: 4 additions & 3 deletions src/bundler/options.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,9 +1087,10 @@ pub const ESMConditions = struct {
var require_condition_map = ConditionsMap.init(allocator);
var style_condition_map = ConditionsMap.init(allocator);

try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len);
try import_condition_map.ensureTotalCapacity(defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len);
try require_condition_map.ensureTotalCapacity(defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len);
const extra: usize = if (allow_addons) 1 else 0;
try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + extra + conditions.len);
try import_condition_map.ensureTotalCapacity(defaults.len + 2 + extra + conditions.len);
try require_condition_map.ensureTotalCapacity(defaults.len + 2 + extra + conditions.len);
try style_condition_map.ensureTotalCapacity(defaults.len + 2 + conditions.len);

import_condition_map.putAssumeCapacity("import", {});
Expand Down
18 changes: 18 additions & 0 deletions test/bundler/bun-build-api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,24 @@
expect(text).toContain(" globalThis.");
});

test("many custom conditions does not crash", async () => {

Check warning on line 814 in test/bundler/bun-build-api.test.ts

View check run for this annotation

Claude / Claude Code Review

Use test.concurrent for consistency with neighboring tests 

Nit: this should be `test.concurrent` to match the neighboring top-level tests in this file (and per `test/CLAUDE.md`'s guidance to prefer concurrent tests when they write files / spawn builds with no shared state).

@claude claude Bot May 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Nit: this should be test.concurrent to match the neighboring top-level tests in this file (and per test/CLAUDE.md's guidance to prefer concurrent tests when they write files / spawn builds with no shared state).

Extended reasoning...

Summary

The new test "many custom conditions does not crash" is declared with plain test(...) rather than test.concurrent(...). This is inconsistent with both the surrounding code in this file and the project's documented testing conventions.

Project convention

test/CLAUDE.md states:

Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with test.concurrent or describe.concurrent unless it's very difficult to make them concurrent.

This test writes files via tempDirWithFiles and runs 18 Bun.build calls — exactly the kind of test the guideline targets.

Local consistency

Looking at the immediate neighbors at the top level of this file (outside the describe("Bun.build") block):

  • test.concurrent("macro with nested object", ...)
  • test.concurrent("regression/NODE_PATHBuild api", ...)
  • test.concurrent("regression/GlobalThis", ...) — directly above
  • describe.concurrent("sourcemap boolean values", ...) — directly below

Every adjacent top-level test uses .concurrent. The only nearby block that does not is describe("tsconfig option"), and that one has a clear reason: it calls process.chdir(), which is process-global and cannot safely run concurrently. The new test has no such constraint.

Why concurrency is safe here

Step-by-step:

  1. The test creates an isolated temp directory with tempDirWithFiles("bun-build-many-conditions", ...) — no path collisions with other tests.
  2. It calls Bun.build({ entrypoints, target, conditions }) with no outdir, so it doesn't write to any shared location.
  3. It does not call process.chdir(), mutate process.env, or touch any other process-global state.
  4. The loop variables (target, n, conditions) are all locals.

There is therefore no obstacle to running it concurrently with the other tests in this file.

Impact

This is purely a consistency / test-suite-throughput nit — the test is functionally correct as written and will pass either way. It just runs sequentially when it could run in parallel with its neighbors, and it diverges from the established pattern at this location in the file.

Fix

-test("many custom conditions does not crash", async () => {
+test.concurrent("many custom conditions does not crash", async () => {

const dir = tempDirWithFiles("bun-build-many-conditions", {
"entry.js": `console.log(1);`,
});

for (const target of ["bun", "browser", "node"] as const) {
for (const n of [1, 4, 6, 8, 12, 20]) {
const conditions = Array.from({ length: n }, (_, i) => `cond${i}`);
const build = await Bun.build({
entrypoints: [join(dir, "entry.js")],
target,
conditions,
});
expect(build.success).toBe(true);
}
}
});
Comment on lines +819 to +830

@coderabbitai coderabbitai Bot May 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Use parameterized cases instead of nested loops for this matrix.

Please convert the (target, n) combinations into parameterized tests so each combo reports as an independent case in failures.

♻️ Suggested refactor 
-test("many custom conditions does not crash", async () => {
+describe.each(["bun", "browser", "node"] as const)("many custom conditions does not crash (%s)", target => {
+  test.concurrent.each([1, 4, 6, 8, 12, 20])("n=%i", async n => {
   const dir = tempDirWithFiles("bun-build-many-conditions", {
     "entry.js": `console.log(1);`,
   });

-  for (const target of ["bun", "browser", "node"] as const) {
-    for (const n of [1, 4, 6, 8, 12, 20]) {
-      const conditions = Array.from({ length: n }, (_, i) => `cond${i}`);
-      const build = await Bun.build({
-        entrypoints: [join(dir, "entry.js")],
-        target,
-        conditions,
-      });
-      expect(build.success).toBe(true);
-    }
-  }
-});
+    const conditions = Array.from({ length: n }, (_, i) => `cond${i}`);
+    const build = await Bun.build({
+      entrypoints: [join(dir, "entry.js")],
+      target,
+      conditions,
+    });
+    expect(build.success).toBe(true);
+  });
+});

As per coding guidelines: "Use describe.each() for parameterized tests".

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/bundler/bun-build-api.test.ts` around lines 819 - 830, Replace the
nested for-loops that iterate over target and n with parameterized test cases
using describe.each or test.each so each (target, n) combo is reported
independently; specifically, create a table/array of tuples for the matrix of
targets ["bun","browser","node"] and ns [1,4,6,8,12,20], then use describe.each
or test.each to iterate those tuples and inside each test call Bun.build with
the same entrypoints and compute conditions = Array.from({ length: n }, (_, i)
=> `cond${i}`) and assert build.success; remove the outer for (const target ...)
and inner for (const n ...) loops and replace them with the parameterized test
harness.


describe.concurrent("sourcemap boolean values", () => {
test("sourcemap: true should work (boolean)", async () => {
const dir = tempDirWithFiles("sourcemap-true-boolean", {
Expand Down
Loading