Conversation
…item outside hot reloading The parser rewrites the global `Response` of a server-side file to an import from "bun:app". It attached a namespace alias to that import item for every output format. Only the dev server module format declares the namespace object. In ES module output each use site printed `import_bun_app.Response`, and nothing declared `import_bun_app`. Attach the alias only for hot reloading. In every other format, register the item in `is_import_item` before the visit, as the JSX runtime import does. The linker adds its own alias when it converts the import for CommonJS or IIFE output. A debug assertion in the import scanner checks that no import clause item has an alias when the bundler links. Co-authored-by: Zack Radisic <56137411+zackradisic@users.noreply.github.com>
|
Status: ready for review. This PR is on top of #44166. Reproduced on 1.4.3-canary.1+367d939d9 (release) and on a debug build of main: printf 'export default function Page() { return new Response("x"); }\nconsole.log(typeof Page());\n' > resp2.ts
bun build --server-components --target=bun ./resp2.ts --outfile out/resp2.js
bun out/resp2.js
|
|
@robobun wake up!! |
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 findings, I also checked the new debug_assert! in src/js_parser/scan/scan_imports.rs against every other parser site that attaches a namespace_alias to an import clause item (p.rs lines 2559, 4408, 4489 and the scanner's own import-star path at scan_imports.rs:548) — each is either gated on hot_module_reloading or runs after the assertion on the same pass, so the assertion should not fire on ordinary ESM/CJS bundles.
Extended reasoning...
The change gates the parser-attached bun:app namespace alias on hot reloading and registers the generated Response symbol in is_import_item otherwise, plus a debug-only assertion and test updates; it touches no security-sensitive surface. Inline findings already cover the behavioral concerns, so this note only records that the other alias-setting sites were checked for the new assertion.
|
@robobun wake up!! |
|
I am here. Build #121331 passed. The review has three optional comments. I will push a test-only commit for two of them (assertions that cannot fail, a run case for iife) and answer in the threads. The third one (the transform also applies to files in |
Four assertions looked for text that the fixture source has too, for example `Response.json(`. The old output `import_bun_app.Response.json(` contains that text, so they passed with or without the transform. Each assertion now includes the token before the read. Add a run case for `--format=iife`. The linker converts the import with `__require` there.
|
Updated 10:03 PM PT - Sep 28th, 2026
✅ @robobun, your commit d118a9780cf4ec2cf8c8f15c9f40071a416ce592 passed in 🧪 To try this PR locally: bunx bun-pr 44167That installs a local version of the PR into your bun-44167 --bun |
There was a problem hiding this comment.
I re-reviewed after b3c4049 and found no bugs; the follow-up anchors the flipped assertions to the surrounding source text and adds the --format=iife run case, which addresses the two test nits from the earlier pass. Because this changes how the parser registers a generated import in the server-components path and a maintainer is already in the thread, a human look is still worthwhile.
What was reviewed:
- The removed post-visit
is_import_item.insert(self.response_ref, ())ingenerate_import_stmt_for_bake_response: the onlyis_import_itemreaders that run after that point (dynamic-import destructure locals in parse_entry.rs andimport_items_for_namespace) never seeresponse_ref, so nothing under hot reloading loses the entry. - The new
debug_assert!inImportScanner::scan: every other parser site that attaches anamespace_aliasto a clause item (jsx/generated imports, default and named clause items) is already gated onhot_module_reloading, and theWAS_ORIGINALLY_REQUIREsite aliases the namespace ref, not an item, so the assertion cannot fire on existing paths. features.hot_module_reloadingis set only forInternalBakeDevnon-runtime sources (ParseTask.rs), matching the const generic the scan is instantiated with.
Extended reasoning...
The change gates the parser-attached namespace_alias on the server-components Response import to hot-reloading mode and otherwise registers the symbol in is_import_item before the visit, plus a debug assertion in the import scanner and test updates that run built esm/cjs/iife/minified/split output. It touches the JS parser and bundler output shape for bun build --server-components and --app; no security-sensitive surface. The source change is small and traced through its consumers, and the latest commit resolved the two test-quality nits from the prior review, but it alters codegen in a core parser path and a maintainer comment whose content is not visible to this run precedes the last push, so approval is left to a human.
Keep the first field doc as it was. State the namespace ref in two lines and the assertion in one.
There was a problem hiding this comment.
Re-reviewed after the two follow-up commits and found no bugs; the earlier inline nits are addressed in the code (anchored assertions plus not.toContain("import_bun_app"), and an iife run case). Since this changes how the parser classifies a generated import symbol, a maintainer look at the parser hunk is still worthwhile.
What was reviewed:
- The HMR-only
namespace_alias.import_record_indexpatch ingenerate_import_stmt_for_bake_responsewas already gated onhot_module_reloadingbefore this PR, so the now-absent alias in non-HMR mode is never unwrapped. - The scanner
debug_assert!is only live in thescan::<_, _, false>instantiation, which is the branch taken exactly whenhot_module_reloadingis off, matching the parser gate. - The synthetic import is emitted only when
response_ref.use_count_estimate > 0, so the non-HMR import scanner cannot elide it as unused. - The new
production.test.tscase follows the sibling test's build/read pattern in the same file.
Extended reasoning...
The diff touches src/js_parser/p.rs (parser init and the bun:app import synthesis), a debug assertion in src/js_parser/scan/scan_imports.rs, and two bake test files; it changes how the generated Response symbol is registered (namespace alias only under HMR, otherwise a plain import item) and no security-sensitive surface is involved. The third review pass found no new bugs and the follow-up commits address the previously posted nits. The change is small but alters symbol classification in the JS parser used by every server-components build, and a maintainer comment whose content is not visible here exists on the thread, so a human look is warranted rather than an automated approval.
Problem
bun build --server-components --target=bunandbun build --appwrite ES modules that readimport_bun_app.Responseand throwReferenceError: import_bun_app is not defined. With--minifyand two readers the load fails:SyntaxError: Cannot declare an imported binding name twice: 'Response'.P::prepare_for_visit_pass(src/js_parser/p.rs:3508) gives the generatedResponseimport item anamespace_aliasin every output format. Only the dev server format declares that namespace.Fix
hot_module_reloading. Otherwise register the item inis_import_itembefore the visit. Adebug_assert!inImportScanner::scanchecks the rule.import { Response } from "bun:app".test/bake/dev/response-to-bake-response.test.ts(11 of 17 tests fail without the fix),production.test.ts, 16 bundler suites.Co-authored-bytrailer. The gate is a port of the hunk in SSG prod #23157.Background
namespace_aliasmakes the printer writenamespace.namefor a read. A symbol inis_import_itembecomes anE::ImportIdentifier, which the linker and the renamers rebind.Downsides
node_modulesincluded, now runs with thebun:appclass. Afetch()result is not an instance of it. CommonJS output and the dev server already do..text: +0 B.Notes
Stack
This PR is on top of #44166. Without that PR, built code that calls
Response.redirect()orResponse.render()reaches a crash in thebun:appclass (Segmentation fault at address 0x0) where it had aReferenceErrorbefore. The new run cases forredirectandrenderfail without it.Reproduction
Before:
ReferenceError: import_bun_app is not defined, exit code 1. After: printsobject.bun build --appwith the React framework and a page that runsnew Response("x")exits 1 before the fix (ReferenceError: e is not definedatpages/index.tsx,eis the minified namespace name). After the fix it renders the page.Reach
--appis refused on a build that is not a canary build.node_modulesare included (src/bundler/ParseTask.rs, theserver_componentsmode has nois_node_module()check).Why the alias was there
handle_identifierturns a read into anE::ImportIdentifierin two cases: the symbol has anamespace_alias, or the symbol is inis_import_item. The first version of the transform used the alias for this, for every format.jsx_importusesis_import_itemfor the same job, andgenerate_import_stmtadds the alias only under hot reloading. This change makes theResponseimport follow that rule. The two arms are exclusive: under hot reloading every read returns from the alias branch, so the map entry has no reader there.The
--format=cjstest proves the registration before the visit. Without it, the read stays anE::Identifier, the linker cannot rebind it, and the CommonJS output reads the globalResponse(the test then printstrue true undefined). For CommonJS and IIFE output the linker adds its own alias and declares the namespace.The
debug_assert!sees only an alias that the parser put on an item of an import clause. It does not see a bareE::Identifierread of a generated symbol.Output formats
bun build,--splitting,--compile,--compile --bytecode --format=esm,bun build --app)A read of
Responsein dead code that the printer keeps (if (false) { switch (new Response("x")) { case 1: } }) printedimport_bun_app.Responsewith no import in esm, cjs and iife. It now printsResponse.Measurements
Debug builds for the counts (gdb breakpoints with counters). Release builds (
--profile=release, one checkout path for both) for sizes and instruction text. The base is main 36cd151, the change is the parser commit alone on that base.P<false, false>) and +32 B (P<true, false>, inlined inParser::_parse::<true>), generate_import_stmt_for_bake_response -105 B (llvm-nm). No other function changes size. The sum of the function sizes is -42 B.handle_identifier, 2 ofImportScanner::scan::<_, false, true>, and 2 ofP::to_ast, which holds the inlinedscan::<_, false, false>.import_bun_app.ResponsebecomesResponse(43 to 60 bytes each).Suites run on the debug build
test/bake/dev/response-to-bake-response.test.ts: 17 pass. The same file withBUN_JSC_validateExceptionChecks=1and LeakSanitizer, as the ASAN lane runs it: 17 pass.test/bake/dev/production.test.ts,test/bake/dev/react-response.test.ts,test/bake/dev/bundle.test.ts,test/bake/app-options.test.ts: 49 pass, 0 fail (--timeout 180000).esbuild/default,importstar,importstar_ts,ts,dce,splitting,extra,bundler_edgecase,bundler_jsx,bundler_cjs,bundler_cjs2esm,bundler_minify,bundler_splitting,bundler_dynamic_import_dce,bundler_npm,cli): 1681 pass, 0 fail. The new assertion did not fire.Test placement
The
bun build --apptest is inproduction.test.ts. That file is intest/no-validate-exceptions.txt:bun build --appstops underBUN_JSC_validateExceptionChecks=1with or without this change (#41185). On a debug build each test of that file needs--timeout, because a production build takes more than 5 s there.Self-review
Response.redirect()crashes where it threw beforeredirectandrenderadded.--apptest aborts on the ASAN lane in a file with exception validationproduction.test.ts.bun:appclass does in built outputCo-authored-bytrailerhot_module_reloadinghunk of #23157 (src/ast/P.zig).The review on this PR had three more comments. The assertions now include the token before each read, so each one fails on the old output. A run case for
--format=iifeis added. A--compilecase is not added: the compile step takes 6 to 13 s on a debug build, which is more than the default timeout of a local test run.Not changed here
ResponseunderemitDecoratorMetadatareads the global class and adds no import. A read inside a directevaladds no import.Responseofbun:appis not the global class.fetch(),Response.json(),Response.error()andclone()return the global class, sox instanceof Responseis false for them in a transformed file.Object.keys(new Response("x"))has 9 entries, andBun.inspect(new Response("x"))prints<null />. The cause is insrc/jsc/bindings/JSBakeResponse.cpp. No PR has this yet.bun build --server-componentswith no--target, and a"use client"file with no framework: bun build --server-components: require the bun target and reject directives without a framework #38046."use server"module panics withTODO: registerServerReference: bundler: report a "use server" module as a build error instead of aborting #42826, bundler: implement server-side wrap for "use server" modules #33589.registerClientReferencesymbols are bare identifiers, which the new assertion does not see: bundler: one React Fast Refresh contract for .jsx and .tsx, add reactFastRefresh.importSource #42010."use client"module with no separate SSR graph aborts the build: bake: do not abort on a "use client" module without a separate SSR graph #42885.bun build --appwith a custom framework panics withRuntime file not found: issue bun build --app panics with "Runtime file not found" for any custom Bake.Framework #32142, PR bake: fix "Runtime file not found" panic in production builds of custom frameworks #32143.--format=iifedoes not call a wrapped CommonJS entry point: bundler: call the wrapped entry point in iife output #37843.export * from "<external>"reads an undeclared name in ES module output (ReferenceError: node_path is not defined): bundler: name the namespace of an external export * in a non-entry module #41220.generate_import_stmt_for_bake_responsecan become a call of a shared emitter. That change has no effect on behaviour and must bring its own size number.to_lazy_export_astruns theResponseblock for JSON, TOML and text sources, where nothing is visited.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/dev/production.test.ts