Repository navigation
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 4:03 AM PT - Sep 6th, 2026
❌ @robobun, your commit c6f6667 has 3 failures in
The baseline build contains instructions not available on Static scan violations
|
There was a problem hiding this comment.
I reviewed this and found no bugs. Because it changes runtime semantics for a valid TS syntax form (import {} from 'x' no longer executes the module), a maintainer should confirm the compat trade-off is intended.
What was reviewed:
- Confirmed the
T::TOpenBracearm only fires with no default binding;import foo, {}routes throughT::TIdentifier → T::TCommaand keeps the default (parse_stmt.rs:1641). - Grepped for
had_type_only_imports— no remaining readers; the field removal is complete. - Checked the existing
default/ImportFormsWithNoBundletest that assertsimport {} from './b'is kept — it uses a.jsentry, so JS mode remains unaffected. preserve_unused_imports_tsis hardcodedfalseat every set site, so noverbatimModuleSyntax-style interaction is reachable today.
Extended reasoning...
Overview
This PR changes the TypeScript parser to drop import {} from 'x' at parse time, matching tsc's default emit and (per the PR description) esbuild. Previously bun kept this as a side-effect import because scan_imports.rs only elides when found_imports is true, and an already-empty clause never sets that flag. The fix removes the had_type_only_imports bookkeeping on ImportClause (parser.rs, parse_import_export.rs) and simplifies the drop condition in parse_stmt.rs to import_clause.items.is_empty() under IS_TYPESCRIPT_ENABLED. Tests in transpiler.test.js cover TS elision, JS preservation, default, {} retention, and a spawned runtime check that a side-effecting module is not evaluated.
Security risks
None. This is a parser control-flow change with no I/O, allocation, or trust-boundary implications. The dropped path (S::TypeScript {}) is the same one already used for import { type T } from 'x' and import type {…}.
Level of scrutiny
Medium-high. The diff is small and mechanically clean, but it is a user-visible semantic change to every .ts file bun runs: code that wrote import {} from './polyfill' and observed side effects will silently stop observing them. REVIEW.md flags both "for ported code, the reference implementation is the spec" and "never silently break existing users" as review-blocking concerns, so a maintainer should sign off on the compat call rather than an automated approval.
Other factors
I traced adjacent paths: the import foo, {} form goes through the T::TIdentifier → T::TComma → T::TOpenBrace branch at parse_stmt.rs:1641, which does not check items.is_empty(), so the default binding is preserved as tested. JS mode is gated out by IS_TYPESCRIPT_ENABLED, and the existing default/ImportFormsWithNoBundle test that asserts import {} from './b' is kept uses a .js entry, so it is unaffected. The preserve_unused_imports_ts option (which would map to importsNotUsedAsValues: "preserve" / verbatimModuleSyntax) is hardcoded to false at every set site today, so there is no currently-reachable configuration under which this parse-time drop conflicts with a "preserve" setting — but if that option is ever wired up, import {} from 'x' would still be dropped before scan_imports sees it, which is worth a maintainer being aware of. The new tests follow harness conventions (tempDir with using, bunEnv/bunExe, concurrent pipe drain, stderr/stdout asserted before exitCode) and would fail on the previous release.
|
Closing this PR. #40822 already covers the same case and does it correctly. This PR does not. With
The diff is right in four rows and wrong in the fifth, where main is right today. #40822 drops an empty |
Problem
.tsfile,import {} from "./polyfill.ts"is kept and the module runs. tsc and esbuild drop it. The same sources behave differently underbun runand under a tsc or esbuild build.import "x"form is a side-effect import. bun already dropsimport { type T } from "x", but an empty clause fell through to the side-effect path (parse_stmt.rs, theT::TOpenBracearm oft_import).Fix
S::TypeScriptand emits nothing.import foo, {} from "x"is unchanged, since the default binding decides it.import {} from "x"still runs the module, as esbuild does.had_type_only_importsfield ofImportClausehad no other reader, so it is removed.test/bundler/transpiler/transpiler.test.js(newan import clause with no bindings is dropped like tsc doestest, fails on 1.4.3). Alsotest/bundler/esbuild/ts.test.ts,test/bundler/esbuild/importstar_ts.test.ts,test/bundler/esbuild/tsconfig.test.ts,test/bundler/bundler_edgecase.test.ts,test/js/bun/typescript/.Background
S::Importstores only the items, so the decision moves to the parser, next to the existing type-only elision.Notes
Repro from the report:
tsc 6.0.3 + node and esbuild 0.28.1:
polyfilled: false. bun 1.4.3:polyfilled: true. bun with this change:polyfilled: false.Bun.Transpileroutput for the covered forms (loaderts):