Repository navigation
Conversation
get_source_code() let an onBeforeParse plugin write its loader into a local that was dropped on return, so run_with_source_code() parsed the plugin's output with the loader implied by the file extension. Store the settled loader back on the task. When the plugin moves a file into or out of the copy-for-bundling loaders, redo the asset bookkeeping the enqueue sites did for the original loader once the parse result arrives, so the asset is written and a converted module keeps its side effects.
|
Warning Review limit reached
Next review available in: 3 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 (4)
Comment |
|
Status: fix pushed, waiting on CI. Reproduced on the released binary with the new |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adjusts bundler graph bookkeeping (estimated_file_loader_count, side_effects) in on_parse_task_complete based on a loader that can now change mid-parse, a human look from someone familiar with the bundler's invariants would still be worthwhile.
What was reviewed:
- Traced
task.loaderwrite-back throughrun_with_source_code— no-op when no plugin runs (same value re-derived);dataurl→js/css stays outsideshould_copy_for_bundlingso the new comparison is a no-op there too. - Checked
additional_filesconsumers (Chunk.rs:734/877, LinkerContext.rs:2471) — they key on the asset's own source index, whichprocess_files_to_copypopulates onceestimated_file_loader_count > 0, so the JS→file path works without an importer-side push; not decrementing the count on file→JS is harmless (only used as a> 0gate). - New
native_plugin.cchooks:malloc'd source is paired with acompilation_ctx_freeand the first test asserts the free counter.
Extended reasoning...
Overview
Two small Rust changes plus test coverage. get_source_code in ParseTask.rs now writes the (possibly plugin-mutated) loader local back into task.loader on the success path, so the second parse stage picks it up instead of re-deriving from the extension. on_parse_task_complete in bundle_v2.rs compares the loader stored in the graph at enqueue time against the one the parse actually used; when a native plugin moved the file into or out of the copy-for-bundling class it re-applies the same bookkeeping the enqueue sites do (bump estimated_file_loader_count, set NoSideEffectsPureData) or restores result.side_effects for the reverse direction. Four new C hooks in the test plugin and six new tests exercise replaced-source + loader, loader-only overrides, file→JS side-effect retention, and JS→file asset emission.
Security risks
None identified. The change threads an already-validated Loader enum value between two stages of the same parse task and adjusts a counter/flag on the bundler thread. No new untrusted-input surface.
Level of scrutiny
Medium-high. The Rust diff is ~25 lines and mechanically simple, but it sits in on_parse_task_complete, the convergence point for every parse result, and touches invariants (estimated_file_loader_count, per-file side_effects, additional_files) that are set at enqueue time on the assumption the loader won't change afterward. I traced the consumers and believe the fix is complete — asset path resolution reads additional_files[asset_source_index] which process_files_to_copy fills in, not the importer-keyed SourceIndex push that the enqueue sites do — but the importer-side list is left asymmetric (stale SourceIndex entry on file→JS; no entry on JS→file), and confirming that no other consumer depends on it is worth a human eye.
Other factors
The PR description is unusually thorough: it names the regressing PR (#17577), explains why each half of the fix is needed by describing the failure with only ParseTask.rs applied, verifies USE_SYSTEM_BUN=1 fails all six tests, and runs the wider bundler suites. It also flags the interacting open issue #37095 (BunLoader enum offset in the public header). The multi-agent bug hunt found nothing. Tests follow harness conventions (concurrent pipe drain, await using, exit code asserted last). Given the subtlety of the graph bookkeeping I'm deferring rather than approving.
|
Updated 10:48 AM PT - Aug 13th, 2026
✅ @robobun, your commit f84d3b62833519de5d9f44e5f1bc5ce2f5686b0c passed in 🧪 To try this PR locally: bunx bun-pr 38172That installs a local version of the PR into your bun-38172 --bun |
Problem
build.onBeforeParsewith anapiModule) can replace a file's source and setOnBeforeParseResult.loaderto say how the replacement should be parsed. This is the documented way to compile e.g..mdxinto JSX (handle.set_output_source_code(code, BunLoader::BUN_LOADER_JSX)in docs/bundler/plugins.mdx;packages/bun-build-mdx-rsdoes exactly that).Bun.builduses the replaced source but ignores the loader: the file is still processed with the loader implied by its extension.export default 42;with the JS loader fordata.foo(unknown extension, so thefileloader). The build succeeds, emitsdata-<hash>.fooas an asset whose contents are literallyexport default 42;, and the bundle containsvar data_default = "./data-<hash>.foo". Setting only the loader (keeping the source,set_output_loaderin bun-native-plugin-rs) is ignored the same way.get_source_code(src/bundler/ParseTask.rs:2216) computes the loader into a local,OnBeforeParsePlugin::runwrites the plugin's choice into that local (ParseTask.rs:2149), andget_source_codereturns only the file contents.run_with_source_code(ParseTask.rs:2288) then derives the loader fromtask.loaderagain, which still holds the extension's loader. Before the split both halves shared one local, so the plugin's loader used to take effect.Fix
get_source_codestores the loader it settled on back intotask.loader, sorun_with_source_codeparses with it and reports it inSuccess.loader(which is what the graph records). Every enqueue site already setstask.loader, so when no plugin runs this writes back the same value.on_parse_task_completecompares the loader the file was enqueued with against the one the parse used. If a plugin moved the file into or out of the copy-for-bundling loaders (file,napi,wasm,sqlite), it redoes the bookkeeping the enqueue sites did for the original loader (bundle_v2.rs:6723-6734): bumpsestimated_file_loader_countand marks the file pure data when it became an asset, or restores the parse task's own side-effect flag when it stopped being one. Both were needed once the loader started taking effect:.jsimport to thefileloader in a build with no other assets leftestimated_file_loader_countat 0, soprocess_files_to_copy(bundle_v2.rs:4175) never ran and chunk generation hitassertion failed: !additional_files.is_empty()inappend_isolated_hashes_for_imported_chunks(release: "Internal error: missing asset file");.foo(fileloader) into JS left the enqueue-timeNoSideEffectsPureDatamark in place, soimport "./x.foo"was tree-shaken away along with the module's side effects (LinkerContext.rs:2809-2813). A JSonLoadplugin returningloader: "js"for the same file keeps them, because for JS plugins the bookkeeping is deferred until the loader is known.dataurland parsed as js/css/json, all outside the copy class; JSonLoadplugins update the graph's loader before the parse task runs).BUN_LOADER_TOMLthroughBUN_LOADER_TEXTinbundler_plugin.hand bun-native-plugin-rs are one off from the runtime's numbering sincejsoncwas inserted; Delete the schema::api mirror types and bun_api; one loader numbering across Rust/C++/JS #37095 corrects them. The constants the new tests use (JS,JSON,FILE) agree in both numberings. Until Delete the schema::api mirror types and bun_api; one loader numbering across Rust/C++/JS #37095 lands, a plugin built against the stale header that asks for one of those six loaders gets the neighbouring loader instead of, as before this change, being ignored.result loaderblock in test/bundler/native-plugin.test.ts (six tests: replaced source + JS loader for an import and for an entry point, loader-only override to JS and to JSON,file-> JS keeping side effects, JS ->fileemitting the asset), driven by four new hooks in test/bundler/native_plugin.cc.USE_SYSTEM_BUN=1 bun test test/bundler/native-plugin.test.ts -t "result loader": 6 fail (asset emitted / text loader applied / side effect dropped / no asset).bun bd test test/bundler/native-plugin.test.ts: 25 pass, 1 skip (pre-existing ASAN skip).console.logmissing from the bundle), which is what motivated the bundle_v2.rs part.bun bd teston bundler_loader, bundler_files, bundler_plugin, bundler_plugin_chain, bundler_edgecase, bundler_html, bun-build-api, esbuild/loader, esbuild/dce: all pass exceptBun.build can be called thousands of times in one process, which hit its 180s ceiling under the debug+ASAN build in this container (400 builds x 500 modules) and is unrelated.Background
js,ts,json,text,file, ...). It is normally chosen from the file extension when the import is resolved and stored both on theParseTaskand in the graph'sinput_filescolumn.Loader::should_copy_for_bundling):file,napi,wasm,sqlite. Such a file is not parsed; its module becomesexport default "<unique key>"and the bytes are written to the outdir as an asset byprocess_files_to_copy, which only runs ifestimated_file_loader_countis non-zero. The enqueue sites increment that counter and mark the fileNoSideEffectsPureDataas soon as they pick such a loader.SideEffectson an input file: anything other thanHasSideEffectstells the tree shaker that a bareimport "x"of that file can be dropped.onBeforeParsenative plugins run on the parse worker thread, inside the parse task, unlike JSonLoadplugins which answer on the bundler thread before the parse task is scheduled. That is why the loader they choose is only visible to the bundler thread when the parse result comes back, inon_parse_task_complete.get_source_code(reads the file or runs the native plugin; may run on the I/O pool) hands aCacheEntryof bytes torun_with_source_code(parses; runs on the worker pool). TheParseTaskitself is the only state shared between the two stages.