Conversation
…only
"export { type foo }" was being erased entirely when every specifier was
type-only, which loses the ESM module marker. Downstream tools and
runtimes then see a script instead of a module (affecting top-level
this, strict mode, etc). esbuild and tsc both emit "export {}" here.
The early return to S::TypeScript in the non-"from" branch is removed so
an empty S::ExportClause is emitted instead. The "from" branch still
drops the whole re-export since there is nothing to import.
Also fixes "import { type as }" (a type-only import of the identifier
"as") which was falling through without marking the clause as type-only
and leaking a bare "import 'mod'" into the output.
|
Reproduced with Tests pass locally: CI build 73455: the only red lanes are |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
|
Updated 7:11 PM PT - Jul 15th, 2026
❌ @robobun, your commit 8b8edc3 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34251That installs a local version of the PR into your bun-34251 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Neither of the suggested issues is fixed by this change:
Leaving the PR body as-is. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped parser fix that aligns type-only export {}/import {} erasure with esbuild/tsc.
What was reviewed:
parse_stmt.rs: removing theS::TypeScriptearly-return falls through to the same emptyS::ExportClausenode that a literalexport {}already produces, so no new print/visit shape;has_es_module_syntax = truenow correctly fires.parse_import_export.rs: tracedimport { type as }throughparse_import_clause— the addedelsecovers the only fall-through case (next token is}or,);had_type_only_exportsis still consumed by thefrompath.- Test matrix covers the
as/as as/from/mixed variants and updates the one existing assertion whose expected output changed.
Extended reasoning...
Overview
Two localized parser changes plus test coverage:
src/js_parser/parse/parse_stmt.rs(t_export,TOpenBracearm): removes theIS_TYPESCRIPT_ENABLED && clauses.is_empty() && had_type_only_exportsearly-return toS::TypeScript {}for the non-fromform, soexport { type x }now falls through to an emptyS::ExportClauseand prints asexport {};(the ESM module marker). Theexport { type x } from "mod"path retains its drop-entirely behavior.src/js_parser/parse/parse_import_export.rs(parse_import_clause): adds the missingelsearm forimport { type as }(i.e.,typefollowed byasfollowed by}/,), which previously fell through without pushing an item and without settinghad_type_only_imports, causingt_importto emit a bareimport "mod";.test/bundler/transpiler/transpiler.test.js: expands the "type only exports" block with a variant matrix and updates the one pre-existing assertion (export { type x }→"") to the new expected output.
Security risks
None. Pure syntactic transform in the TS parser; no I/O, no user-controlled allocation sizes, no FFI/JSC interaction.
Level of scrutiny
Low-medium. The diff is small (net −9/+21 in src/), mechanically follows esbuild's reference implementation (cited in the PR), and the resulting AST node (S::ExportClause { items: [] }) is already the exact shape produced by a user-written export {}, so the printer and visit passes are unchanged and already exercised. I traced the import { type as } branch by hand: after consuming type then as, the next token is }/,, so both existing if guards fail — the new else is the only sink. had_type_only_exports remains live via the from path at parse_stmt.rs:1261, so no dead code is introduced.
Other factors
- robobun confirms tests pass on the debug build and fail on
USE_SYSTEM_BUN=1. - The test additions cover the sibling forms called out in CLAUDE.md ("cover the variant matrix"):
type as,type x as y, multiple type specifiers, unicode-escapedtype,fromvs non-from, and the mixedimport { type as, x }case. - No prior human review comments; no CODEOWNER on these paths.
|
@robobun what if there are other export statements that do not get eliminated? would we end up with something like |
|
Yes, and that exactly matches esbuild: Same for tsc is smarter here and drops the empty clause when there is already another ESM statement in the file (in either order), so matching tsc would mean a whole-file check rather than a per-statement decision. esbuild has never done that optimization, and bun already leaves a literal user-written So this PR just makes |
|
@robobun we should not match esbuild here. if there's already an esm export statement we should be smart like tsc and drop the empty one. the empty statement is harmful because it hurt bundle size and parsing/lexing |
… exists
When type-stripping leaves an "export {}" clause behind, drop it if the
output already contains another export statement (or top-level await)
that marks the file as ESM. This matches tsc and avoids dead weight in
the output; esbuild always keeps the redundant clause.
The decision is order-independent: a pre-visit scan records which
export forms definitely survive, and the visit pass tracks remaining
export clauses so an empty clause appearing before a non-empty one is
still dropped while never losing the marker entirely.
JavaScript input is untouched.
|
Done in 1b7d02d. Empty The check runs in TypeScript only; a literal Implementation: a pre-visit scan records exports that definitely survive ( The two |
"export default Foo" where Foo is a local interface/type alias is dropped
during visit, so it is not safe to count in the pre-visit scan as a
surviving export. The pre-scan now only counts an SExportDefault whose
value is not a bare identifier; the visit pass sets the flag once the
default actually survives the type-only check.
Also update the cyclic-imports-async-bundler snapshot (debugId only; the
fixture pairs top-level await with a redundant export {} in a .ts entry).
There was a problem hiding this comment.
The fix in 8b8edc3 correctly addresses my earlier finding — I re-traced the four ordering combinations of export default <ident> × export {} and the ESM marker is now preserved in each. Leaving final sign-off to dylan-conway since he's already engaged on the redundant-clause optimization.
What was reviewed:
- Verified the pre-scan now excludes bare-identifier
SExportDefaultand thats_export_defaultsetshas_nonempty_export_stmtonly after passing the local-type-name drop check. - Confirmed
SNamespacein the pre-scan is safe (type-only namespaces becomeS::TypeScriptat parse time as stated). - Checked the
remaining_export_clauses_with_itemsdecrement ordering vs.remove_for_tree_shaking— the only lossy interaction requiresreplace_exports+ tree-shaking, an internal HMR path.
Extended reasoning...
Overview
This PR changes TypeScript type-stripping in the JS parser so that export { type Foo } lowers to export {} (preserving the ESM module marker) instead of vanishing entirely. A second commit, requested by dylan-conway, adds a whole-file optimization that drops the resulting empty export {} clause when another surviving export or top-level await already marks the file as ESM. A third commit fixes a regression I flagged where export default <TypeName> was being counted as "definitely surviving" in the pre-visit scan even though visit can drop it.
Files touched: two new fields on the P parser struct, a pre-visit scan in parse_entry.rs, drop logic in visit_stmt.rs::s_export_clause, a flag-set in s_export_default, removal of an early return in parse_stmt.rs, and a one-line missing-else fix in parse_import_export.rs for import { type as }. Three snapshot files update only their debugId hash.
Security risks
None. This is TypeScript type-erasure output shaping; no untrusted-input parsing surface changes, no allocation sizing, no FFI.
Level of scrutiny
High — this is the JS parser, on the hot path for every .ts file Bun touches. The optimization introduces cross-statement state (has_nonempty_export_stmt, remaining_export_clauses_with_items) shared between a pre-visit scan and the visit pass, with an ordering-dependent invariant ("exports counted in the pre-scan must definitely survive visit"). My previous review caught a violation of that invariant for SExportDefault; the fix now excludes bare-identifier defaults from the pre-scan and sets the flag during visit only after the local-type-name check passes. I re-traced the added regression tests and the inverse orderings and they hold.
Other factors
- dylan-conway is actively reviewing and specifically requested the redundant-clause optimization, so he should confirm the final shape matches what he had in mind.
- Test coverage is thorough: 30+ new
expectPrinted_cases intranspiler.test.jscovering source-order permutations, theexport default <TypeName>regression, TLA, and theimport { type as }fix. The PR body shows these fail on main and pass with the fix. - One acknowledged false negative:
export {}; const Foo = 1; export default Foo;keeps the redundant marker because the pre-scan can't knowFoobinds to a value. This is harmless (extra 10 bytes) and matches esbuild. - I noted a theoretical edge case where
remove_for_tree_shakingins_export_clause(which requires the internalreplace_exportsfeature + tree-shaking + all specifiers replaced-and-removed) could drop a clause an earlier empty clause deferred to; not a practical concern for user code.
This branch has not been deployed
2 participants
Problem
When every specifier in an
export { ... }clause istype-only, Bun erased the whole statement:Dropping the statement entirely loses the ESM module marker, so downstream tools and runtimes may treat the output as a script rather than a module (top-level
this, strict mode, etc).A related bug:
import { type as } from "mod"(a type-only import of the identifieras) was falling through a missingelsebranch inparse_import_clauseand leaking a bareimport "mod";into the output instead of being dropped.Fix
parse_stmt.rs: remove the early-return toS::TypeScriptfor the non-fromexport { ... }case and fall through to an emptyS::ExportClause, which prints asexport {};. Theexport { type foo } from "bar"path still drops the whole re-export since there is nothing to import.parse_import_export.rs: add the missingelsebranch forimport { type as }sohad_type_only_importsis set and the statement is dropped.parse_entry.rs/visit_stmt.rs: drop the resultingexport {};clause whenever another export statement (or top-levelawait) already marks the output as ESM, so type-only exports alongside real exports no longer leave a redundant marker behind. This matches tsc; esbuild always keeps the redundant clause. The check is order-independent and TypeScript-only.Verification
test/bundler/transpiler/transpiler.test.js"type only exports" now covers:export { type x }export {};export { type as }export {};export { type x } from 'mod'export const a = 1; export { type B }export const a = 1;export const a = 1;export { type B }; export const a = 1export const a = 1;export const a = 1;export { type A }; export { b }export { b };export { b };export { type A }; export { type B }export {};import { type as } from 'mod'import"mod";The previous assertion
expectPrinted_("export { type x };", "")is updated to expect"export {}". Twobundler_promiseall_deadcodesnapshots update theirdebugIdhash only (fixture entries pairawaitwith a now-redundantexport {};; bundled code is unchanged).[review] gate passed · iteration 2 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file