Repository navigation
bundler: call the wrapper of a CommonJS or lazy-ESM <script src> at its tag in an HTML entry point - #42026
bundler: call the wrapper of a CommonJS or lazy-ESM <script src> at its tag in an HTML entry point#42026robobun wants to merge 8 commits into
Conversation
… point A <script src> that the bundler classifies as CommonJS (top-level this in a UMD header, module.exports behind a typeof guard) or wraps as a lazy ES module was emitted as var require_x = __commonJS(...) / var init_x = __esm(...) with no call, so its body never ran. The HTML AST kept every import record in one statement-less part, so the linker had no import statement to turn into the wrapper call. Give each import record of an HTML file its own part and print the wrapper call (require_x(), init_x(), await init_x()) for that part, at the tag's position in the merged module. Stop registering a __toESM use for HTML importers, and keep the blank line between files when an empty part range sits between them.
|
Updated 12:26 AM PT - Oct 14th, 2026
❌ @robobun, your commit 01ec601 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 42026That installs a local version of the PR into your bun-42026 --bun |
|
Reproduced on 1.4.3 ( d=$(mktemp -d); cd $d
printf '(function (root) { root.LIB = { v: 1 }; })(this);\nconsole.log("a");\n' > a.js
printf 'console.log("m"); if (typeof module === "object") module.exports = {};\n' > m.js
echo 'console.log("b");' > b.js
echo '<!doctype html><html><body><script src="./a.js"></script><script src="./m.js"></script><script src="./b.js"></script></body></html>' > index.html
bun build ./index.html --outdir=out && bun out/index-*.js # prints only "b"The chunk defines |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughHTML bundling now creates ordered synthetic parts for script imports. Live parts emit wrapper calls for wrapped modules, with asynchronous ESM wrappers awaited when required. Linker behavior, chunk formatting, and HTML execution tests were updated. ChangesHTML script bundling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies the main change: invoking wrapped CommonJS and lazy-ESM scripts from HTML entry points. |
| Description check | |
The description is detailed and covers the problem, implementation, verification, limitations, test coverage, and known CI status. It does not use the template headings exactly, but it provides the re… |
Full details: Description check
-
Explanation
The description is detailed and covers the problem, implementation, verification, limitations, test coverage, and known CI status. It does not use the template headings exactly, but it provides the required information.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how the linker structures and prints parts for HTML entry points — core bundler code-generation — a maintainer look would still be worthwhile.
What was reviewed:
append_html_script_wrapper_callsagainstshould_remove_import_export_stmt— thefiles_liveskip for ESM-only and thewrapper_ref.is_empty()guard match; the CJS arm intentionally calls the wrapper directly (no__toESM, noE::RequireString) since nothing binds the namespace, and async ESM uses an inlineawaitviaappend_non_dependency(sequential, document-order) rather thanappend_async_dependency'sPromise.allbatching.ParseTasknow emits one part per import record andtree_shaking_and_code_splittingseeds bits1..parts.len()— consistent with each other; theis_htmlgate ingenerate_code_for_file_in_chunk_jsandscan_imports_and_exportskeeps non-HTML paths untouched.post_process_js_chunknow only setsnewline_before_commenttrue (never clears it) — matches the esbuild behavior the PR cites; snapshot/etag churn traced to the part-range change feeding the chunk hash, not to changed emitted bytes for existing tests.
Extended reasoning...
Overview
This PR fixes a bug where <script src> tags in HTML entry points that resolve to wrapped modules (CommonJS or lazily-initialized ESM) were never executed in the bundled output. The fix restructures the HTML loader's synthetic AST from one part holding all import records to one part per record (src/bundler/ParseTask.rs), seeds all those parts as live in tree-shaking (src/bundler/LinkerContext.rs:934-946), and adds append_html_script_wrapper_calls (src/bundler/LinkerContext.rs:2318-2375) which is invoked from generate_code_for_file_in_chunk_js for HTML sources to print require_foo() / init_foo() / await init_foo() at each tag's document position. Two supporting one-liners: scan_imports_and_exports skips __toESM bookkeeping for HTML importers, and post_process_js_chunk no longer lets an empty compile result clear newline_before_comment. Tests add four itBundled cases covering all wrap kinds and splitting, one standalone compile: true test, and update etag/hash snapshots.
Security risks
None. This is bundler output-generation logic operating on already-parsed ASTs; no new input parsing, no auth/crypto/network surface. The changed code paths do not touch untrusted external data beyond what the bundler already handles.
Level of scrutiny
Moderate-to-high. The bundler linker is ported from esbuild and REVIEW.md treats the reference implementation as the spec. The new function deliberately diverges from should_remove_import_export_stmt in two ways: (1) the CJS arm calls the wrapper ref directly rather than emitting E::RequireString bound to a namespace symbol, and (2) async ESM uses append_non_dependency with an inline E::Await (sequential) rather than append_async_dependency (which batches into Promise.all). Both divergences are justified — nothing reads the namespace, and HTML scripts must run in document order — but they are the kind of "why this and not the alternative" call a maintainer should confirm. The per-tag part structure also interacts with find_imported_parts_in_js_order and chunk hashing; the PR author checked splitting, minify, format=cjs/iife, sourcemaps, and external scripts by hand, but those aren't all under automated test.
Other factors
Test coverage is strong for the fix itself: all CJS classification triggers (UMD this, guarded module.exports, .cjs, require(), "type": "commonjs"), sync and async lazy ESM, and code-splitting with a shared CJS script across two pages. Tests execute the emitted chunk and assert stdout in document order, so they fail on the unfixed build. The PR references two related drafts (#41999, #41982) touching the same Loader::Html block, which suggests ongoing design work here that a maintainer should be aware of. No CODEOWNERS entries cover the changed files. Exit reason was dry_streak, so the bug hunt ran to exhaustion without findings.
The chunk prints wrapped files ahead of flat code, so this is the case where the wrapper call must come from the tag's own part to keep document order. Adapted from the html/commonjs-script-runs-in-place case in #41999.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Consolidation note: #41999 made the same per-tag part change for Checked again with a debug build of this branch: |
…-script-cjs-wrapper-call
The chunk walk on main no longer prints a file part by part for an HTML file, so the wrapper calls printed after every later script. The HTML AST is back to one part for all import records. Once wrap kinds are final, call_wrapped_html_scripts gives that part a require_x() / init_x() / await init_x() statement for each <script src> whose file is wrapped, and ends the part at each call that another record follows. The walk reports every record of an HTML file under its part, so a call prints between the scripts around its tag. HTMLScanner marks script[src] records with HTML_SCRIPT_SRC, so a <link as=worker> file gets no call. A page with no wrapped script keeps its part layout, so its chunk keeps the same bytes and the same content hash: the hash snapshots are back to the values on main.
…-script-cjs-wrapper-call
robobun
changed the title
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.
This revision moves the split into the linker, and the two points from my earlier review are addressed by it: for_each_edge now reports each HTML record under its real part index, and only the script[src] handler sets HTML_SCRIPT_SRC, so a link[as=worker] target is never called (both covered by the new tests). Beyond the inline note, I also checked that parts_live is seeded after scan_imports_and_exports (so the split parts are live), that step 6 reads parts_len after the split (so each new part gets the wrapper_ref symbol use needed for the cross-chunk import under --splitting), and that the detached arena ref follows the same pattern as bundle_v2.rs since add_part_to_file only pushes to a per-file part list.
Extended reasoning...
The change touches the bundler linker (part splitting in call_wrapped_html_scripts, the chunk visitor's HTML edge walk, step 6 of scan_imports_and_exports) plus a new ImportRecordFlags bit threaded through HTMLScanner; no security-sensitive surface. The inline nit on test coverage stands, and the linker ordering logic is intricate enough that a human familiar with findAllImportedPartsInJSOrder should still look, so this is a defer rather than an approve.
…y, source maps and an import()ed page call_wrapped_html_scripts no longer treats a wrapped HTML file as a special case: each call ends its part, and a wrapped file prints its parts in order inside its wrapper anyway. New cases run the built chunk for --format=iife, --format=cjs, the minifier and an external source map, for a lazily-initialized module that has no wrapper to call, and for a page that a script loads with import().
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; the points raised on earlier versions (worker-link records getting a call, the dead guards, and the missing iife/cjs/minify/sourcemap coverage) are addressed in the current code and tests. Because this adds a part-splitting step inside the linker's step 6 plus a new chunk-walk branch, a human look from someone who knows the linker would still be worthwhile.
What was reviewed:
- Traced
call_wrapped_html_scriptsend to end: it runs before the step-6 part loop recomputesparts_len, so the new parts get thewrapper_refsymbol use and wrapper-part dependency; every new part carries at least one import record, so theparts_liveseeding marks all of them. - Checked the
for_each_edgeHTML branch against the walk inplace: each part's records split a run at that part index, so the call part lands after the files it imports and before the next tag's script; theis_broken_bycheck cannot trip because wrapper parts stayHoisted. is_async_or_has_async_dependencyis final beforescan_imports_and_exports(validate_tla/propagate_async_dependencies run first), and the wrapper part defaults tocan_be_removed_if_unused: false, so a lazy-ESMinit_xis live whenever its page is.- All three
HTMLProcessorHandlerimplementors take the newflagsparameter;HTML_SCRIPT_SRC(1<<19) does not collide with existing bits.
Extended reasoning...
The change touches the bundler linker (src/bundler/LinkerContext.rs, scanImportsAndExports.rs, findAllImportedPartsInJSOrder.rs), the HTML scanner, and the import-record flag set, with about 140 lines of Rust and roughly 285 lines of new tests; it touches no security-sensitive surface. It adds a new linker step that splits an HTML file's synthetic part and appends parts via add_part_to_file, uses an unsafe lifetime detach on the bundle arena (a pattern already used elsewhere in the bundler), and changes how the chunk walk orders HTML files. The tests run the emitted chunk and cover CJS and lazy-ESM scripts, iife/cjs/minify/sourcemap, splitting, --compile, and an imported page, and the earlier inline feedback is addressed. The decision to defer rather than approve rests on the complexity of the part-splitting and chunk-order logic, which is not simple or mechanical; no CODEOWNERS entry covers src/bundler.
This branch has not been deployed
2 participants
Problem
bun build ./index.html) never runs a<script src>that the linker wraps. A CommonJS file prints asvar require_a = __commonJS(...)and nothing callsrequire_a(). An__esm-wrapped script is the same. Exit 0, no diagnostic. Triggers:"type": "commonjs",.cjs,require(), a UMD header.ParseTask.rs,Loader::Html). The linker prints a wrapper call only from animportstatement.Fix
call_wrapped_html_scripts(linker step 6) addsrequire_a(),init_a()orawait init_a()to that part for each wrapped<script src>.HTMLScannermarks those records (HTML_SCRIPT_SRC), so a<link as="worker">file gets no call.test/bundler/bundler_html.test.ts(11 cases:iife,cjs, minify, source map, splitting),test/bundler/standalone.test.ts(1). Each runs the built chunk and fails on main.await init_x()order (the current contract), a call for alink[as=worker]file (not a script).Background
<script src>into one JS chunk. The linker wraps a file it cannot inline there: CommonJS in__commonJS(run byrequire_x()), a lazy ES module in__esm(init_x()).Downsides
thislands onexports(Failure to include js script when serving static html #18770 family).size, at 4fb8cf5).Notes
bun build, 81 of 114 did unbundled. No linked user report.b.js,a.js(UMD),m.js(guardedmodule.exports),b2.js:generate_code_for_file_in_chunk_js. Main then rewrote the chunk walk (bundler: print a file where it is imported, not where its bindings are used #44508):for_each_edgereported every record of an HTML file under part 0, so the whole file was placed after its imports and the calls printed after every later script. That version also changed the part ranges of every HTML page: each HTML entry chunk got a new content hash with the same bytes (the snapshot churn inhtml-import-manifest.test.tsandbun-serve-html.test.ts), one 200-bytePartand one allocation per import record, and empty print tasks that needed a newline fix inpost_process_js_chunk. Both designs were built and compared before this rework.ParseTask.rs,postProcessJSChunk.rs,generateCodeForFileInChunkJS.rsand both snapshot files are back to main.scan_imports_and_exportssteps 1 and 2). Step 6 is the first point where it is final, and it is where the per-part wrapper symbol use is registered, so the statement and its symbol use land on the same part.import(), stylesheets, images, an external script, a bare page, a CSS-only page) built six ways (default,--minify,--splitting,--sourcemap=linked,--format=iife,--compile --target=browser): 90 output files, same names and sha256 on the merge base and on this branch, debug and release builds.importstatement per bundled script, with a new name for the JS chunk of every page. It was closed in favor of bundler(splitting): give a shared chunk to an entry point whose order breaks no load-time read #44876, which "leaves every other build unchanged". This PR prints the same calls from the linker and keeps the chunk of every other page byte-identical, name included. If the parse-time shape is preferred, the tests here carry over unchanged.import()is itself a lazily-initialized module. Its scripts never ran either. The calls now print inside the wrapper of the page, in document order (html/script-src-commonjs-in-imported-page).init_x. The page makes no call for it (pure.jsinhtml/script-src-lazy-esm).--format=internal_bake_dev(the dev server's module format) is skipped: it loads a script through its module record and defines norequire_x. Release output for a page with a CommonJS script is byte-identical on the merge base and on this branch. The dev server itself (bun ./index.html) never ran this linker step and was not affected by the bug.scan_imports_and_exportsstep 6 no longer setsWRAP_WITH_TO_ESMfor an HTML importer. It used to surface as an unusedimport { __toESM }in the page's entry chunk under--splitting.importstatements in the HTML AST: the generic path printsvar import_a = __toESM(require_a()), which needs a namespace symbol per tag and the__toESMruntime helper, and tree shaking does not walk an HTML file's part dependencies, so the helper would not be included.<link as="worker">file is merged into the page chunk and runs there (on main today).--format=internal_bake_devwith an HTML entry trips a debug assertion inprint_dev_server_moduleon main.for_each_edge. Instruction counts were not taken:perf,valgrind,bloatyandstraceare not installed where this was measured. Release file size: 105,604,840 to 105,608,936 bytes (+4,096).call_wrapped_html_scripts, and add tests.wrapis in steps 1 and 2 ofscan_imports_and_exports, before step 6); the lazy-ESM test pinsawait init_x()before the next script (that is what a merged module does today, and bundler: start an HTML page's top-level-await scripts without holding back later scripts #42105 is the place to change it); a wrappedlink[as=worker]file keeps an uncalled wrapper (intended: the page must not run a worker file).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/standalone.test.ts