Repository navigation
dev server: an export * after another statement of the same path loses exports of a barrel target - #46229
Conversation
… HMR lowering HMR lowering merges every statement of one import path into the first one and retires the import record of each later statement. A retired record is never resolved. export_star_import_records kept the retired record of a merged `export *`, so the barrel pass never found the target of that `export *`: the target was requested only for the names of the other statement, and a target that is a barrel deferred the rest. Those exports were undefined in the dev server. convert_stmt now writes the record that the merged statement uses into the entry that ImportScanner pushed for the statement. named_imports has the same stale index and is not changed here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughWhen ChangesExport-star deduplication
Suggested reviewers: Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to
|
|
Status Reproduced on 1.4.3 and on main bun bd test test/bake/dev/bundle.test.ts -t "an export star merged"
# main: got: undefined function undefined function undefined A
# this branch: got: function function function function function AThe test passes on 1.4.2. A root import of PR: #46229 |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the last_mut().unwrap() invariant against ImportScanner (src/js_parser/scan/scan_imports.rs:790 pushes the record for the unaliased export * only, immediately before the convert_stmt call at line 844, so the write lands on this statement's own entry) and the case of two export * statements of one path now listing one record twice — every reader of export_star_import_records already handles two records resolving to the same target, so that is not affected.
Extended reasoning...
The change is a four-line fix in the HMR ESM-export lowering pass plus a dev-server test; it touches no security-sensitive surface. The inline finding covers the sibling export * as ns arm, which routes through named_imports rather than export_star_import_records and is the same stale-record class the author lists as deferred to a follow-up PR; a human should weigh whether that split is acceptable.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js_parser/lower/lower_esm_exports_hmr.rs— Dev-server users importing a namespace re-export (export * as ns from 'x') that follows another statement for 'x' in asideEffects: falsebarrel getundefinedforns, while the bareexport *one line below is fixed. The aliased arm at lower_esm_exports_hmr.rs:351 computesdedupedbut never writes the surviving record anywhere;named_imports(scan_imports.rs:778) still names the retired record. Whennsis requested later, barrel_imports.rs:935 un-defers only the retired record and the surviving one stays deferred. Fix: writededuped.import_record_indexinto thenamed_importsentry forst.namespace_refin the aliased arm too, so both SExportStar shapes name a live record. [also at: src/js_parser/lower/lower_esm_exports_hmr.rs:320 - Dev-server users of asideEffects: falsebarrel still readundefinedfor a name re-exported by a secondexport { } fromof a path already imported, after this merge, because only theexport *sibling is repaired. lower_esm_exports_hmr.rs:320 rewritessymbol.namespace_aliasto the surviving record, butp.named_imports(scan_imports.rs:813, :778) keeps the retired record.]Why this was flagged
Trigger: a
sideEffects: falsepackage index withexport { a } from './inner.js'; export * as ns from './inner.js';, served by the dev server, where neitheranornsis requested when the barrel parses andnsis requested by a later-parsed module.deduplicated_importretires the second record (lower_esm_exports_hmr.rs:419-421). The aliased arm at :351-361 pushes a property usingdeduped.namespace_refbut, unlike the bare arm at :370-372, does not repair any record list. ImportScanner recordednamed_imports[namespace_ref].import_record_index = st.import_record_indexat scan_imports.rs:778, the retired one. Whennsis requested,resolve_barrel_export(:899) yields the retired record,un_defer_record(:935) clears its flags and resolves it, but the surviving record that the HMR-loweredimport * asstatement references stays IS_UNUSED|IS_BARREL_DEFERRED, soinner.jsis not loaded for that import andnsreadsundefined. The base behaves the same, but the PR fixes the sibling arm of the samematchand REVIEW.md treats same-class sites as one concern.Verification: pre-existing; acknowledged in diff: the PR description states "Not fixed:
named_importshas the same stale record", and the aliasedexport * as nsarm is exactly anamed_importssite (scan_imports.rs:771-782 recordsimport_record_index: st.import_record_indexfor the aliased star). The base fails by the same route; the new test (test/bake/dev/bundle.test.ts) contains noexport * ascase.
|
The To point The |
…export-star-surviving-record
…tself and shorten the test
export * that HMR lowering mergesexport * after another statement of the same path loses exports of a barrel target
There was a problem hiding this comment.
I reviewed the updated version of this PR and did not find any bugs; since the fix sits in the dev-server barrel optimization path, which has had several ordering-sensitive regressions, a human look at the new commit would still be worthwhile.
What was reviewed:
- Traced that under HMR every
export *still reachesconvert_stmt(no early exit before theSExportStararm), so moving the push out of the scanner drops no records; the non-HMR scan path is unchanged andAstBuildernever fills the list. - Checked each reader of
export_star_import_records(barrel_imports.rs, scanImportsAndExports.rs, bundle_v2.rs, js_printer, and the clientstaticExportNameswalk) for the duplicate indices thestar-twicefixture now produces — all are set/flag/idempotent operations. - Confirmed the latest commit removed the
.last_mut().unwrap()rewrite from the prior version in favor of a plain push.
Extended reasoning...
The change is two production lines in the Rust js_parser (a push in the HMR lowering SExportStar arm and a const-generic gate on the scanner's push) plus one devTest with five fixtures in test/bake/dev/bundle.test.ts. It touches no security-sensitive surface. The logic is small and I traced it to every consumer, but it depends on the interplay of HMR import deduplication, the barrel deferral pass, and dev-server path resolution, an area with repeated prior regressions (#28886, #42737) and known remaining gaps the author lists, and the test could not be executed in this environment, so a human look is still warranted rather than an unattended approve.
|
CI on
Generated by Claude Code |
What does this PR do?
Fixes a regression in 1.4.3, from #42737. The dev server (
Bun.serveHTML routes) gives the appundefinedfor an export, with no build error, when a module has anexport *after another statement of the same path and the target is a"sideEffects": falsebarrel:The first statement can also be an
import, and the module with theexport *can be app code that re-exports a barrel package.@mui/material7 has this shape inesm/index.js(export { default as RadioGroup } from "./RadioGroup/index.js"; export * from "./RadioGroup/index.js";), so a root import ofuseRadioGroupisundefined.Cause
deduplicated_importinsrc/js_parser/lower/lower_esm_exports_hmr.rs) merges every statement of one path into the first one. It marks the import record of each later statementIS_UNUSED, so that record is never resolved.export_star_import_recordsstill listed that record.on_parse_task_completeand the barrel pass (src/bundler/barrel_imports.rs) take the target of anexport *from this list. They found no target, so nothing asked the target for all of its exports, and the target deferred every re-export that no importer named.un_defer_recordclearedIS_UNUSEDon any record. That also revived the merged-away record, and the target was found through it. bundler: un-defer only the records barrel optimization deferred #42737 limits it to the records the barrel pass deferred.Fix
With HMR lowering,
convert_stmtpushes the record that the merged statement uses (deduped.import_record_index) toexport_star_import_records.ImportScannerpushes the statement's own record only without HMR lowering. Every reader of the list now gets a resolved record, including thestarslist the dev server prints for each module.Not changed
Both of these are the same on 1.4.2 and on this branch:
named_importsstill names the merged-away record of anexport { b } fromorexport * as ns from. The name isundefinedwhen it is first requested after the barrel was parsed and its path was deferred.export *asked for all of it defers it again on the next rebuild.How did you verify your code works?
New test in
test/bake/dev/bundle.test.ts:barrel optimization: export star after another statement of the same path.491aaad1@mui/material7.3.11 in Chromium:useRadioGroup,ModalManager,ButtonGroupContext,stackClassesundefinedgot: undefined function undefined undefined undefined. The onefunctionis the control, where theexport *is the first statement of its path.USE_SYSTEM_BUN=1on a system Bun of 1.4.2 or older, which does not have the regression.test/bake/dev/bundle.test.tspasses on the debug build.Generated by Claude Code