Skip to content

fix(bundler): avoid invalid syntax in void import() transformation - #24750

Closed
robobun wants to merge 1 commit into
mainfrom
claude/fix-dynamic-import-void-syntax
Closed

robobun wants to merge 1 commit into
mainfrom
claude/fix-dynamic-import-void-syntax

Conversation

@robobun

@robobun robobun commented Nov 16, 2025 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes #24709

When bundling code with void import() (side-effect only dynamic imports), the bundler was generating invalid JavaScript syntax like .then(() => ) instead of .then(() => void 0).

Before:

function main() {
  Promise.resolve().then(() => );  // ❌ Invalid syntax
}

After:

function main() {
  Promise.resolve().then(() => void 0);  // ✅ Valid syntax
}

Root Cause

When the bundler transforms internal dynamic imports, it wraps them in Promise.resolve().then(() => <content>). However, when the imported module has no exports or wrapper to call (e.g., pure side-effect imports that get tree-shaken), nothing was being printed in the callback body, resulting in invalid syntax.

Solution

The fix tracks whether any content was printed in the dynamic import body and ensures we print void 0 when nothing else needs to be printed, resulting in valid JavaScript syntax.

Test Plan

  • Added regression test in test/regression/issue/24709.test.ts
  • Test fails with USE_SYSTEM_BUN=1 bun test (reproduces the bug)
  • Test passes with bun bd test (verifies the fix)
  • Manually verified the generated code is syntactically valid

🤖 Generated with Claude Code

Fixes #24709

When bundling code with `void import()` (side-effect only dynamic imports),
the bundler was generating invalid JavaScript syntax like `.then(() => )`
instead of `.then(() => void 0)`.

This happened when the bundler transformed internal dynamic imports but had
no content to put in the .then() callback (e.g., when the module had no
exports or wrapper to call).

The fix tracks whether any content was printed in the dynamic import body
and ensures we print `void 0` when nothing else needs to be printed,
resulting in valid syntax.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@robobun

robobun commented Nov 16, 2025 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:42 PM PT - Nov 15th, 2025

❌ Your commit e2a926d5 has 3 failures in Build #31826 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 24750

That installs a local version of the PR into your bun-24750 executable, so you can run:

bun-24750 --bun

@coderabbitai

coderabbitai Bot commented Nov 16, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

The PR fixes incorrect code transformation during dynamic imports by tracking printed content within import bodies and ensuring syntactically valid expressions. When no content is generated, void 0 is output to maintain validity. Dev server handling and ESM/CommonJS interop are adapted accordingly. A regression test is added.

Changes

Cohort / File(s) Summary
Dynamic import printing fix
src/js_printer.zig
Introduces has_dynamic_content flag to track printed content. Switches dev server paths to use hmr_ref with .require() calls. Refines non-dynamic paths to conditionally print wrapper calls and exports wrapping based on wrapper_ref and exports_ref validity. Outputs void 0 when dynamic imports have no printed content to ensure syntactic validity.
Regression test
test/regression/issue/24709.test.ts
Adds test validating JavaScript output for void import() patterns. Verifies compiled output contains valid .then(() => void 0) syntax instead of invalid .then(() => ), and confirms build exits successfully.

Possibly related PRs

  • oven-sh/bun#23803: Modifies dynamic import and interop wrapping logic in js_printer.zig for import() result printing and to-ESM/__toESM wrapping.

Pre-merge checks

✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately and concisely describes the main fix: avoiding invalid syntax in void import() transformations during bundling.
Description check ✅ Passed The PR description follows the template structure with clear sections explaining what the PR does, root cause, solution, and test plan verification.
Linked Issues check ✅ Passed The PR directly addresses issue #24709 by fixing the generation of invalid syntax in void import() transformations, ensuring .then(() => void 0) is generated instead of .then(() => ).
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the dynamic import void syntax issue: modifications to js_printer.zig track printed content and add void 0 fallback, while the test validates the fix.

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 2cb8d4e and e2a926d.

📒 Files selected for processing (2)
  • src/js_printer.zig (1 hunks)
  • test/regression/issue/24709.test.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
📚 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/24709.test.ts
📚 Learning: 2025-10-18T23:43:42.502Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23817
File: src/js/node/test.ts:282-282
Timestamp: 2025-10-18T23:43:42.502Z
Learning: In the Bun repository, the error code generation script (generate-node-errors.ts) always runs during the build process. When reviewing code that uses error code intrinsics like $ERR_TEST_FAILURE, $ERR_INVALID_ARG_TYPE, etc., do not ask to verify whether the generation script has been run or will run, as it is automatically executed.

Applied to files:

  • test/regression/issue/24709.test.ts
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.

Applied to files:

  • test/regression/issue/24709.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/24709.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.

Applied to files:

  • test/regression/issue/24709.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/24709.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/regression/issue/24709.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Applied to files:

  • test/regression/issue/24709.test.ts
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.

Applied to files:

  • test/regression/issue/24709.test.ts
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.

Applied to files:

  • test/regression/issue/24709.test.ts
🧬 Code graph analysis (1)
test/regression/issue/24709.test.ts (1)
test/harness.ts (2)
  • tempDir (277-284)
  • bunExe (102-105)
🔇 Additional comments (2)
src/js_printer.zig (1)

1677-1731: Dynamic import has_dynamic_content tracking and void 0 fallback look correct; consider asserting wrap_with_to_esm invariants

This block cleanly fixes the original bug: for internal dynamic imports where both meta.wrapper_ref and meta.exports_ref end up unused (e.g. void import("./foo") after DCE), has_dynamic_content becomes false and the record.kind == .dynamic guard appends void 0, yielding .then(() => void 0) instead of invalid .then(() => ). The dev-server branch using hmr_ref.require(path) also correctly marks has_dynamic_content = true, so dynamic imports in internal_bake_dev mode stay valid.

One thing to sanity‑check: the !wrap_with_to_esm condition assumes that whenever record.wrap_with_to_esm is true, the block will always emit some inner expression (via wrapper/exports/HMR), otherwise we could theoretically end up with Promise.resolve().then(() => __toESM(, 1)). If that invariant isn’t already guaranteed by the linker, it may be safer to either:

  • broaden the fallback to also cover the wrap_with_to_esm case (e.g. emit __toESM(void 0, 1) when has_dynamic_content is false), or
  • add a debug assertion like bun.debugAssert(!wrap_with_to_esm or has_dynamic_content); near this block.

If you’re confident the invariant holds in all current call sites, the existing code is fine, but an assertion would make future regressions easier to catch.

test/regression/issue/24709.test.ts (1)

1-35: Regression test thoroughly exercises the void dynamic import transform

The test setup (temp project with bug.ts, await using proc = Bun.spawn(...) with bunExe, bunEnv, and disposable cwd) matches existing harness patterns and ensures proper cleanup. Assertions on absence of .then(() => ), presence of .then(() => void 0), a non‑empty arrow body via regex, and exitCode === 0 together give good coverage of the bug and the intended fix without over‑constraining the exact emitted code shape.

Optionally, if this ever flakes, you could include stderr in the failure message for easier diagnosis, but functionally this test looks solid as is.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Nov 16, 2025

Copy link
Copy Markdown
Collaborator Author

Closing as duplicate - this was already fixed in PR #24710

@robobun robobun closed this Nov 16, 2025
@robobun robobun reopened this Nov 16, 2025
@robobun robobun added the slop label Nov 16, 2025
@github-actions github-actions Bot changed the title fix(bundler): avoid invalid syntax in void import() transformation ai slop Nov 16, 2025
@github-actions github-actions Bot closed this Nov 16, 2025
@github-actions
github-actions Bot deleted the claude/fix-dynamic-import-void-syntax branch November 16, 2025 03:40
@robobun robobun changed the title ai slop fix(bundler): avoid invalid syntax in void import() transformation Nov 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect code transformation during build for dynamic imports

1 participant