Repository navigation
Conversation
Output for bun carries a `// @bun` pragma that makes bun load it as an ES module in every format but cjs, so lowering an entry point's `import.meta.main` to `__require.main == module` in an iife for bun references bindings that do not exist there. Decide whether the lowering applies with one (format, target) predicate shared by the parser (which records the `__require` use the lowering needs) and the printer, and give the printer the file's own target so an entry point whose `#!/usr/bin/env bun` hashbang switched it to bun, and therefore got the pragma, is printed the same way.
|
Warning Review limit reached
Next review available in: 2 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 (8)
Comment |
|
Updated 10:00 AM PT - Aug 13th, 2026
❌ @robobun, your commit ddfa91a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38139That installs a local version of the PR into your bun-38139 --bun |
|
Status: reproduced on the released binary ( |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Not adding |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is well-reasoned and the tests are thorough, but since it changes the printer's per-file target source in LinkerContext (bundle target → ast.target) and adjusts codegen across output formats, a maintainer sign-off would be worthwhile.
Checked that printer.options.target is only read in the EImportMetaMain arm when bundling: true (the third read at lib.rs:7489 is gated on !bundling), so the ast.target swap is scoped to exactly this expression.
Verified !keeps_import_meta_main widens lower_import_meta_main to cjs/iife/bake-dev, but record_usage_of_runtime_require is a no-op there because auto_polyfill_require is esm-only today — parser output is unchanged on this branch.
Confirmed ast.target is set from topts.target for every non-hashbang file (ParseTask.rs:2662), and non-entry files inline import.meta.main before the printer, so only the entry point's target is observable.
Extended reasoning...
Overview
Fixes import.meta.main printing for iife-format output targeting bun, and for the entry-with-#!/usr/bin/env bun case when the bundle target is something else. Adds Format::keeps_import_meta_main(target) as the single source of truth, uses it in the printer's EImportMetaMain arm and in ParseTask where the parser option is set, renames lower_import_meta_main_for_node_js → lower_import_meta_main (5 mechanical sites), and switches LinkerContext's printer options from the bundle-wide target to the file's ast.target. Four new itBundled tests cover the reported iife+bun case, the hashbang esm/iife cases, and pin the cjs+bun boundary.
Security risks
None. Pure codegen decision for how a single AST node prints; no untrusted-input parsing, no allocation/lifetime changes, no FFI.
Level of scrutiny
Moderate-high. The bundler's linker and printer are hot, correctness-critical paths, and the self.options.target → ast.target swap in LinkerContext changes a per-bundle input to a per-file one. I verified the PR's claim that this only affects EImportMetaMain: the printer's other read of options.target (lib.rs:7489) is behind !printer.options.bundling, and LinkerContext sets bundling: true. print_with_writer's positional target argument (which drives IS_BUN_PLATFORM) was already ast.target before this PR, so the change actually removes an existing inconsistency between the two target sources rather than introducing one.
I also checked the widened lower_import_meta_main condition. It now fires for cjs, iife-non-bun, and bake-dev entry points (previously node-only), but record_usage_of_runtime_require is gated on auto_polyfill_require, which ParseTask sets only for esm output — so the parser produces identical output on this branch for every widened case. The predicate is exhaustive over Format, and the esm/node-esm/iife-browser/cjs behaviors all match the pre-PR truth table.
Other factors
Tests are strong: each new case both asserts the exact printed expression (capture), asserts absence of require/module in the output where they'd be undefined at runtime, checks the pragma prefix, and executes the bundle expecting true true false false. The cjs+bun test pins the unchanged boundary. The description lists which existing suites were re-run. Non-entry files can't reach this printer arm (their import.meta.main is inlined during visit), so per-file target divergence for non-entry files in the same chunk is not a concern. Given the scope touches the linker's per-file printer setup, deferring for a maintainer glance rather than auto-approving.
|
Confirmed the |
Related: #24540 (that report is the dynamic
require()case, fixed by #38077; this PR is theimport.meta.maincase, which #38077 leaves out). Does not close it on its own.Problem
import.meta.main(orrequire.main === module, which the parser folds to the same node) bundled withbun build --target=bun --format=iifethrows when the output is loaded:ReferenceError: __require is not definedtoday,ReferenceError: module is not definedonce bundler: define __require in iife output #38077 defines__requirefor iife output.ExprData::EImportMetaMaininsrc/js_printer/lib.rs:2993) keepsimport.meta.mainonly for esm output and lowers it to<require>.main == modulefor every other format. That lowering assumes the output is loaded as a CommonJS script. Output for bun starts with a// @bunpragma (src/bundler/linker_context/postProcessJSChunk.rs:352), and Bun loads a// @bunfile as an ES module unless the pragma also says@bun-cjs(cjs output only):import.meta.mainworks in that file and neithermodulenorrequireexist.#!/usr/bin/env bunwhen the bundle is built for another target:src/bundler/ParseTask.rs:2403switches that file to target bun and its chunk gets the pragma, but the printer was given the bundle's target (src/bundler/LinkerContext.rs:2222). With--target=nodethe esm output is__require.main == __require.modulewith__requirenever defined, because the parser (which used the file's target) did not record the use that the printer (which used the bundle's) relied on.Fix
src/options_types/bundle_enums.rs:Format::keeps_import_meta_main(target)is the one place that says whetherimport.meta.mainstays as written: esm output except for node (node has noimport.meta.main), and iife output for bun. cjs and the dev server format are lowered as before.src/js_printer/lib.rsdecides with that predicate. The only output it changes is iife for bun, which now containsimport.meta.mainitself; esm, cjs and iife for browser/node print exactly what they printed before.src/bundler/ParseTask.rsuses the same predicate to set the parser option that records the runtime__requireuse the lowering needs. The option is renamed fromlower_import_meta_main_for_node_jstolower_import_meta_mainbecause it is no longer node specific. Recording is a no-op unless the output imports the runtime's__require(auto_polyfill_require, esm only today), so on this branch the parser's output is unchanged; once bundler: define __require in iife output #38077 turns that on for iife, this keeps an iife for bun from carrying an unusedvar __require = import.meta.require;while browser/node iife get the__requirethey print. (bundler: define __require in iife output #38077 adds an|| !output_format.is_esm()to the sameifinp.rs; whichever lands second can drop that, since this option covers it.)src/bundler/LinkerContext.rspasses the file's ownast.targetto the printer instead of the bundle's. Inside the bundler the printer readstargetonly in theEImportMetaMainarm, so this changes nothing except for the hashbang case, where the file's target is the one the pragma (and the parser) already follow. The same function already passedast.targetas the positional target ofprint_with_writer(LinkerContext.rs:2276); the options struct was the one place still using the bundle's. Therequire.main == modulelowering stays for cjs output for bun: that output is wrapped in a CommonJS function by the@bun-cjspragma, where it is correct.test/bundler/bundler_edgecase.test.ts:edgecase/ImportMetaMainIIFETargetBun(the report),edgecase/ImportMetaMainBunHashbangTargetNodeandedgecase/ImportMetaMainIIFEBunHashbangTargetNode(the hashbang entry, esm and iife) check the printed expression, that the output contains norequire/moduleat all, and run the bundle under bun expectingtrue true false false. All three fail on the released binary (they print__require.main == module/__require.main == __require.module).edgecase/ImportMetaMainCJSTargetBunpins the cjs boundary and passes before and after.bundler_edgecase.test.ts(DeepImportDiamondDAGtimes out locally, a 20k-module stress test unrelated to this),bundler_cjs.test.ts,bundler_banner.test.ts(hashbang handling),minify/RequireMainToImportMetaMain,compile/ImportMetaMain,esbuild/default.test.ts -t "DefineImportMeta|IIFE",test/js/bun/resolve/import-meta.test.jsandbake/dev/bundle.test.ts -t import.meta.main.__commonJSand never called (bundler: call the wrapped entry point in iife output #37843), and browser/node iife output still lacks the__requirebinding it prints (bundler: define __require in iife output #38077). With this change a bun-target iife printsimport.meta.maininside such a wrapper as well, so it works once bundler: call the wrapped entry point in iife output #37843 calls it.Background
import.meta.mainis a Bun feature: true in the module the process was started with. Outside esm output the bundler lowers it to arequire.main == modulecomparison, which only makes sense where the output is evaluated as a CommonJS module (so thatrequireandmoduleexist). Non-entry files are folded tofalseby the parser;--compilefolds the entry point totrue; only a plainbun buildentry point reaches the printer.// @bunpragma: the linker puts it at the top of every chunk built for bun. When Bun later runs such a file it skips transpiling it and loads it as is, as an ES module;// @bun @bun-cjs(cjs output) instead tells it the file is the CommonJS function wrapper that follows. This is why iife output for bun has to keepimport.meta.mainwhile cjs output for bun has to lower it.__require: for output formats that import the bundler's runtime helper, references to a dynamicrequire(including this lowering) are printed as the runtime's__requiresymbol, and the part defining it is only kept in the output if some file recorded a use of it during parsing. The parser does not know the target, soParseTaskpasses target-dependent decisions to it as options;lower_import_meta_mainis such an option.ParseTaskparses the first entry point with target bun when its first line is exactly#!/usr/bin/env bun, whatever--targetsays, and stores that on the file's AST; the pragma is emitted from the entry point's AST target.Repro on the released binary
Both print
truewith this change; the output isimport.meta.mainin each case. A// @bunfile printingtypeof module, typeof requiregivesundefined undefined.