Skip to content

Commit 6c7e972

Browse files
authored
fix(bundler): barrel optimization drops exports used by dynamic import (#27695)
## What does this PR do? Fixes invalid JS output when a `sideEffects: false` barrel is used by both a **static named import** and a **dynamic `import()`** in the same build with `--splitting --format=esm`. ## Root Cause `src/bundler/barrel_imports.zig` had a heuristic that skipped escalating `requested_exports` to `.all` for `import()` when the target already had a partial entry from a static import: ```zig } else if (ir.kind == .dynamic) { // Only escalate to .all if no prior requests exist for this target. if (!this.requested_exports.contains(target)) { try this.requested_exports.put(this.allocator(), target, .all); } } ``` This is unsafe — `await import()` returns the **full module namespace** at runtime. The consumer can destructure or access any export and we can't statically determine which ones. ## Failure chain 1. Static `import { a } from "barrel"` seeds `requested_exports[barrel] = .partial{"a"}` 2. Dynamic `import("barrel")` is ignored because `requested_exports.contains(barrel)` is true 3. Barrel optimization marks `export { b } from "./b.js"` as `is_unused` → `source_index` cleared 4. Code splitting makes the barrel a chunk entry point (dynamic import → own chunk) 5. Linker can't resolve the barrel's re-export symbol for `b` → uses the unbound re-export ref directly 6. Output: `export { b2 as b }` where `b2` has no declaration → **SyntaxError at runtime** With `--bytecode --compile` this manifests as an `OutputFileListBuilder` assertion (`total_insertions != output_files.items.len`) because JSC rejects the chunk during bytecode generation, leaving an unfilled slot. ## Real-world trigger `@smithy/credential-provider-imds` is a `sideEffects: false` barrel used by: - `@aws-sdk/credential-providers` — static: `import { fromInstanceMetadata } from "@smithy/credential-provider-imds"` - `@smithy/util-defaults-mode-node` — dynamic: `const { getInstanceMetadataEndpoint, httpRequest } = await import("@smithy/credential-provider-imds")` ## Fix Dynamic import **always** marks the target as `.all` in both Phase 1 seeding and Phase 2 BFS. This is the same treatment as `require()`. ## Tests - **New:** `barrel/DynamicImportWithStaticImportSameTarget` — repros the bug with `--splitting`, verifies both exports work at runtime. Fails on system bun with `SyntaxError: Exported binding 'b' needs to refer to a top-level declared variable.` - **Updated:** `barrel/DynamicImportInSubmodule` — was testing that a fire-and-forget `import()` doesn't force unused submodules to load. That optimization can't be safely applied, so the test now verifies the conservative (correct) behavior: all barrel exports are preserved.
1 parent 32edef7 commit 6c7e972

File tree

2 files changed

+100
-15
lines changed

2 files changed

+100
-15
lines changed

src/bundler/barrel_imports.zig

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -301,10 +301,9 @@ pub fn scheduleBarrelDeferredImports(this: *BundleV2, result: *ParseTask.Result.
301301
// Handle import records without named bindings (not in named_imports).
302302
// - `import "x"` (bare statement): tree-shakeable with sideEffects: false — skip.
303303
// - `require("x")`: synchronous, needs full module — always mark as .all.
304-
// - `import("x")`: mark as .all ONLY if the barrel has no prior requests,
305-
// meaning this is the sole reference. If the barrel already has a .partial
306-
// entry from a static import, the dynamic import is likely a secondary
307-
// (possibly circular) reference and should not escalate requirements.
304+
// - `import("x")`: returns the full module namespace at runtime — consumer
305+
// can destructure or access any export. Must mark as .all. We cannot
306+
// safely assume which exports will be used.
308307
for (file_import_records.slice(), 0..) |ir, idx| {
309308
const target = if (ir.source_index.isValid())
310309
ir.source_index.get()
@@ -319,10 +318,9 @@ pub fn scheduleBarrelDeferredImports(this: *BundleV2, result: *ParseTask.Result.
319318
const gop = try this.requested_exports.getOrPut(this.allocator(), target);
320319
gop.value_ptr.* = .all;
321320
} else if (ir.kind == .dynamic) {
322-
// Only escalate to .all if no prior requests exist for this target.
323-
if (!this.requested_exports.contains(target)) {
324-
try this.requested_exports.put(this.allocator(), target, .all);
325-
}
321+
// import() returns the full module namespace — must preserve all exports.
322+
const gop = try this.requested_exports.getOrPut(this.allocator(), target);
323+
gop.value_ptr.* = .all;
326324
}
327325
}
328326

@@ -354,8 +352,8 @@ pub fn scheduleBarrelDeferredImports(this: *BundleV2, result: *ParseTask.Result.
354352
}
355353
}
356354

357-
// Add bare require/dynamic-import targets to BFS as star imports (matching
358-
// the seeding logic above — require always, dynamic only when sole reference).
355+
// Add bare require/dynamic-import targets to BFS as star imports — both
356+
// always need the full namespace.
359357
for (file_import_records.slice(), 0..) |ir, idx| {
360358
const target = if (ir.source_index.isValid())
361359
ir.source_index.get()
@@ -366,8 +364,7 @@ pub fn scheduleBarrelDeferredImports(this: *BundleV2, result: *ParseTask.Result.
366364
if (ir.flags.is_internal) continue;
367365
if (named_ir_indices.contains(@intCast(idx))) continue;
368366
if (ir.flags.was_originally_bare_import) continue;
369-
const is_all = if (this.requested_exports.get(target)) |re| re == .all else false;
370-
const should_add = ir.kind == .require or (ir.kind == .dynamic and is_all);
367+
const should_add = ir.kind == .require or ir.kind == .dynamic;
371368
if (should_add) {
372369
try queue.append(queue_alloc, .{ .barrel_source_index = target, .alias = "", .is_star = true });
373370
}

test/bundler/bundler_barrel.test.ts

Lines changed: 91 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -541,7 +541,9 @@ describe("bundler", () => {
541541
});
542542

543543
// --- Ported from Rolldown: dynamic-import-entry ---
544-
// A submodule dynamically imports the barrel back
544+
// A submodule dynamically imports the barrel back. import() returns the full
545+
// module namespace — all barrel exports must be preserved, even if the
546+
// import() result is discarded (we can't statically prove it isn't used).
545547

546548
itBundled("barrel/DynamicImportInSubmodule", {
547549
files: {
@@ -562,17 +564,103 @@ describe("bundler", () => {
562564
export const a = 'dyn-a';
563565
import('./index.js');
564566
`,
565-
// b.js has a syntax error — only a is imported, so b should be skipped
566567
"/node_modules/dynlib/b.js": /* js */ `
567-
export const b = <<<SYNTAX_ERROR>>>;
568+
export const b = 'dyn-b';
568569
`,
569570
},
570571
outdir: "/out",
571572
onAfterBundle(api) {
572573
api.expectFile("/out/entry.js").toContain("dyn-a");
574+
// b must be included — import() needs the full namespace
575+
api.expectFile("/out/entry.js").toContain("dyn-b");
573576
},
574577
});
575578

579+
// Dynamic import returns the full namespace at runtime — consumer can access any export.
580+
// When a file also has a static named import of the same barrel, the barrel
581+
// optimization must not drop exports the dynamic import might use.
582+
// Previously, the dynamic import was ignored if a static import already seeded
583+
// requested_exports, producing invalid JS (export clause referencing undeclared symbol).
584+
itBundled("barrel/DynamicImportWithStaticImportSameTarget", {
585+
files: {
586+
"/entry.js": /* js */ `
587+
import { a } from "barrel";
588+
console.log(a);
589+
const run = async () => {
590+
const { b } = await import("barrel");
591+
console.log(b);
592+
};
593+
run();
594+
`,
595+
"/node_modules/barrel/package.json": JSON.stringify({
596+
name: "barrel",
597+
main: "./index.js",
598+
sideEffects: false,
599+
}),
600+
"/node_modules/barrel/index.js": /* js */ `
601+
export { a } from "./a.js";
602+
export { b } from "./b.js";
603+
`,
604+
"/node_modules/barrel/a.js": /* js */ `
605+
export const a = "A";
606+
`,
607+
"/node_modules/barrel/b.js": /* js */ `
608+
export const b = "B";
609+
`,
610+
},
611+
splitting: true,
612+
format: "esm",
613+
target: "bun",
614+
outdir: "/out",
615+
run: {
616+
stdout: "A\nB",
617+
},
618+
});
619+
620+
// Same as above but static and dynamic importers are in separate files.
621+
// This was parse-order dependent — if the static importer's
622+
// scheduleBarrelDeferredImports ran first, it seeded .partial and the dynamic
623+
// importer's escalation was skipped. Now import() always escalates to .all.
624+
itBundled("barrel/DynamicImportWithStaticImportSeparateFiles", {
625+
files: {
626+
"/static-user.js": /* js */ `
627+
import { a } from "barrel2";
628+
console.log(a);
629+
`,
630+
"/dynamic-user.js": /* js */ `
631+
const run = async () => {
632+
const { b } = await import("barrel2");
633+
console.log(b);
634+
};
635+
run();
636+
`,
637+
"/node_modules/barrel2/package.json": JSON.stringify({
638+
name: "barrel2",
639+
main: "./index.js",
640+
sideEffects: false,
641+
}),
642+
"/node_modules/barrel2/index.js": /* js */ `
643+
export { a } from "./a.js";
644+
export { b } from "./b.js";
645+
`,
646+
"/node_modules/barrel2/a.js": /* js */ `
647+
export const a = "A";
648+
`,
649+
"/node_modules/barrel2/b.js": /* js */ `
650+
export const b = "B";
651+
`,
652+
},
653+
entryPoints: ["/static-user.js", "/dynamic-user.js"],
654+
splitting: true,
655+
format: "esm",
656+
target: "bun",
657+
outdir: "/out",
658+
run: [
659+
{ file: "/out/static-user.js", stdout: "A" },
660+
{ file: "/out/dynamic-user.js", stdout: "B" },
661+
],
662+
});
663+
576664
// --- Ported from Rolldown: multiple-entries ---
577665
// Multiple entry points that each import different things from barrels
578666

0 commit comments

Comments
 (0)