bundler: detect module.exports=require() redirect through a constant-test if/else - #35611
bundler: detect module.exports=require() redirect through a constant-test if/else#35611robobun wants to merge 3 commits into
Conversation
WalkthroughChangesThe parser unwraps side-effect-free constant CommonJS redirect detection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Updated 8:50 PM PT - Aug 5th, 2026
✅ @robobun, your commit 79f05d6793229bef276ff91cc86ae63bbf15413e passed in 🧪 To try this PR locally: bunx bun-pr 35611That installs a local version of the PR into your bun-35611 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I couldn't find any issues here — the unwrap is tightly guarded and semantically equivalent to what --minify-syntax already does at the same point — but this is new pattern-matching in the bundler's CJS→ESM redirect path that changes emitted output, so it's worth a human look.
What was reviewed:
- Confirmed
Expr::as_bool()only matchesEBoolean/EBranchBooleanliterals, so the test expression is side-effect-free. - The unwrap only reassigns the local
stmt;use_count_estimate == 1,import_records.len() == 1, and the exactmodule.exports = require(...)match all still gate the redirect, so a non-matching live branch falls through unchanged. - Checked the
live = None/SEmpty/ multi-statement-block cases — all leavestmtas theSIf, so no redirect fires (conservative). - Tests cover both branch polarities, braced/unbraced bodies, and the react ESM-namespace rewrite path;
cjs2esm: trueasserts the wrapper is gone.
Extended reasoning...
Overview
This PR extends the module.exports = require(...) redirect detector in src/js_parser/parse/parse_entry.rs to see through a residual if (<bool literal>) {} else { X } wrapper. The pattern arises when process.env.NODE_ENV is folded to a constant and DCE empties the dead branch, but --minify-syntax isn't on to flatten the SIf itself. The fix is a ~40-line unwrap that, when the test is a boolean literal and the dead branch is empty, reassigns the local stmt to the live branch's single statement before the existing SExpr match runs. Four new itBundled tests cover the matrix.
Security risks
None. This is compile-time AST pattern matching in the bundler; no untrusted input parsing, I/O, or auth surface is touched.
Level of scrutiny
Medium-high. The change is small and the safety argument is compelling: with --minify-syntax the SIf is already flattened to its live branch before this code runs, and the existing ModuleExportsBasedOnNodeEnv{Production,Development} tests (with minifySyntax: true) exercise the redirect on identical parser state. All downstream guards — use_count_estimate == 1, the exact EBinary(BinAssign, EDot(module.exports), ERequireString|EIdentifier) shape, and import_records.len() == 1 for the namespace path — are unchanged and still gate the redirect after the unwrap. as_bool() only returns Some for EBoolean/EBranchBoolean, so the test is guaranteed side-effect-free. If the live branch doesn't match, stmt is either left as the SIf or set to a non-SExpr, the match fails, and behavior is identical to before.
That said, this is the bundler's CJS→ESM output path — a wrong redirect here drops a __commonJS wrapper and rewires callers, which would break real bundles. It's not a mechanical or config-level change, so per the approval guidelines it warrants a human sign-off rather than auto-approval.
Other factors
The tests are well-constructed: cjs2esm: true asserts no __commonJS( in output, the react test additionally asserts no require_react wrapper and no dead-branch file in the bundle, and all four run the output to verify the correct branch's value is used. The existing ModuleExportsEqualsRuntimeCondition test (non-constant if test) still asserts the wrapper is kept, guarding against over-eager unwrapping.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/bundler_cjs2esm.test.ts`:
- Around line 203-207: In the regression-test comments for
test/bundler/bundler_cjs2esm.test.ts at lines 203-207, retain only the issue URL
and remove the explanatory text; at lines 266-266, 294-296, 325-326, and
360-362, remove the explanatory comments entirely. Leave the associated tests
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a8e9813-4ae3-4959-9eaa-8c9ebfd3fac2
📒 Files selected for processing (2)
src/js_parser/parse/parse_entry.rstest/bundler/bundler_cjs2esm.test.ts
There was a problem hiding this comment.
I didn't find any correctness issues, but this changes bundler output for the CJS→ESM redirect path and adds ~50 lines of new pattern-matching in parse_entry.rs, so it warrants a human look. The comment-cop flag on the 15-line explanatory comment at parse_entry.rs:1253-1267 is also still outstanding.
What was reviewed:
- Confirmed
SideEffects::to_booleanis the same predicates_ifuses to mark the dead branch, and returns!okwhendead_code_eliminationis off — so the unwrap can never disagree with DCE. - Checked that a non-empty dead branch (hoisted
var, surviving fn decl, else-if with truthy outer test) failsdead_is_emptyand falls through unchanged. - Verified the redirect path returns a fresh
Ast::empty_in— the discardediftest/shell can't leak stale state;ModuleExportsEqualsRuntimeConditionstill guards the non-constant case.
Extended reasoning...
Overview
Adds a while let SIf unwrap loop in parse_entry.rs just before the existing module.exports = require(...) redirect matcher. When the file's single non-trivial statement is an if whose test folds to a side-effect-free constant and whose dead branch has already been emptied by DCE, the loop peels down to the live branch's single statement so the existing SExpr/EBinary match can fire. Six new itBundled cases in bundler_cjs2esm.test.ts cover prod/dev NODE_ENV, unbraced bodies, numeric --define, else-if chains, and the real react/index.js shape with target: bun.
Security risks
None. This is a bundler output optimization; no untrusted-input parsing surface is added, and the only effect of a false positive would be dropping a __commonJS wrapper (behaviorally covered by the run.stdout assertions).
Level of scrutiny
Medium-high. The change is small and well-guarded, but it alters bundled output for every package using the NODE_ENV redirect pattern (react, react-dom, scheduler, etc.). The guards look right — to_boolean is exactly the predicate visit_stmt::s_if used to decide which branch to visit under is_control_flow_dead, and the dead_is_empty check means anything DCE had to preserve (hoisted var, fn decls) blocks the unwrap. On success the whole AST is replaced with Ast::empty_in(arena) + redirect_import_record_index, so no stale statement references survive. I traced the else-if-chain iteration and the loop terminates (each step descends to a strict AST child). One conservative gap I noticed but didn't flag as a bug: when the outer test is truthy and the else is itself an SIf, dead_is_empty is false and the unwrap bails — that's strictly no worse than today.
Other factors
The comment-cop action flagged the paragraph comment at line 1267 and CodeRabbit flagged the multi-line test comments; neither has been addressed since the last push. The tests themselves are solid — each asserts both runtime output and (for the react case) absence of require_react/react.production.min.js in the bundle, and the pre-existing ModuleExportsEqualsRuntimeCondition test still pins the non-constant case to unhandled. Given this affects the bundler's redirect fast-path across the ecosystem's most common package shape, a maintainer sign-off is appropriate.
There was a problem hiding this comment.
I didn't find any bugs, but this touches bundler module-redirect detection where the correctness relies on DCE invariants (emptied dead branch, decremented module use count, import-record state), so it's worth a human look.
What was reviewed:
- Verified
SideEffects::to_booleangates ondead_code_eliminationand requiresok && NoSideEffects, so the peel only fires when the visitor already emptied the dead arm. - Traced the else-if chain case through the
whileloop; the single-statement live-branch guard and empty-dead-branch guard hold at each level. - Confirmed
cjs2esm: truein the new tests asserts zero__commonJSwrappers and that the react test'sonAfterBundleasserts the specific wrapper is gone. - The unresolved comment-cop note on
parse_entry.rs:1256re-fired after the trim; the current 4-line comment reads as a legitimate why-comment, likely a bot false positive.
Extended reasoning...
Overview
The PR extends the module.exports = require(...) redirect detector in parse_entry.rs to see through a constant-test if/else before matching the SExpr. Without --minify-syntax, DCE empties the dead arm but leaves the SIf shell (if (false) {} else { module.exports = ... }), which previously blocked the redirect. The new while loop peels SIf layers when SideEffects::to_boolean proves the test is a side-effect-free constant, the dead arm is empty (None/SEmpty/empty SBlock), and the live arm reduces to exactly one statement. Six new itBundled cases in bundler_cjs2esm.test.ts cover prod/dev NODE_ENV, unbraced arms, a numeric __DEV__ define, an else-if chain, and the real React index.js shape.
Security risks
None. This is a compile-time bundler optimization operating on already-parsed AST; no untrusted input parsing, no I/O, no allocation on external size fields.
Level of scrutiny
Moderate-to-high. The change is small (~35 lines of Rust) and additive — it only pre-processes stmt before the existing match — but it lives in a bundler correctness path. If the guards are wrong, the redirect could fire on a module whose if shell is semantically load-bearing, silently dropping code. The guards look sound (to_boolean itself returns ok=false when DCE is off; hoisted var/function in the dead arm would leave it non-empty so the peel bails; the downstream use_count_estimate == 1 gate is unchanged), and the test matrix is thorough. But the reasoning depends on DCE having already normalized use counts and emptied the branch in the visitor pass, which is the kind of cross-pass invariant a maintainer should confirm.
Other factors
- The change reuses
SideEffects::to_boolean, the same predicate theSIfvisitor uses to pick the dead arm, so the two sites cannot disagree on which branch is live. - The
cjs2esm: trueharness flag does a strong assertion (zero__commonJSwrappers in output), and each test also runs the bundle to verify runtime behavior. - An unresolved comment-cop bot note remains on
parse_entry.rs:1256after the author trimmed the comment to four lines; the remaining comment states a non-obvious invariant and looks like a false positive, but the author may want to address or dismiss it. - No prior
claude[bot]review on this PR.
There was a problem hiding this comment.
No bugs found, but this touches the bundler's CJS→ESM redirect detection — worth a quick human sanity-check on the DCE invariant it relies on.
What was reviewed:
- Confirmed
SideEffects::to_booleanis the same predicate thes_ifvisitor uses (visit_stmt.rs:1699), and returnsok: falsewhen DCE is off — so the unwrap can't fire without DCE having run. - The peeled
stmtis detection-only; on no-match the originalpartsare emitted unchanged, and themodule_refuse-count guard still gates the redirect. - Loop terminates (strictly descends the AST);
dead_is_emptyand single-statement-live-branch guards fall back to old behavior on anything unexpected. - New tests assert both zero
__commonJSwrappers (viacjs2esm: true) and runtime stdout, covering both NODE_ENV values, unbraced bodies, numeric define, else-if chain, and the react namespace-identifier path.
Extended reasoning...
Overview
Adds a ~40-line preprocessing loop in src/js_parser/parse/parse_entry.rs that peels constant-test SIf statements down to their live arm's single statement before the existing module.exports = require(...) redirect matcher runs. This lets the redirect optimization fire on the standard React NODE_ENV switch pattern without --minify-syntax. Six new itBundled cases in test/bundler/bundler_cjs2esm.test.ts cover the variant matrix.
Security risks
None. This is a pure bundler output-shape optimization with no user-controlled inputs beyond source code that already flows through the parser. No auth, crypto, filesystem, or network surface.
Level of scrutiny
Medium-high. The bundler's CJS wrapping logic is a critical correctness path — a false positive here would drop a wrapper around a module that actually needs one, changing runtime semantics. The change is well-guarded: it reuses SideEffects::to_boolean (verified identical to what visit_stmt.rs:1699 uses to pick the dead arm), requires the dead arm to already be empty (proving DCE ran and left nothing behind), requires exactly one statement in the live arm, and only affects the local stmt used for pattern detection — the original parts are emitted unchanged if the downstream SExpr match fails. The existing use_count_estimate == 1 guard on module_ref still applies. The while loop handles else-if chains and terminates because each iteration descends to a strict subtree.
Other factors
Test coverage is strong: cjs2esm: true asserts zero __commonJS wrappers appear in output (the load-bearing assertion for #12726), and run: { stdout } asserts runtime correctness. The variant matrix covers production/development, braced/unbraced, numeric --define, else-if chains, and the react-specific path where require() was already rewritten to a namespace identifier. Bot review comments (comment length) have been addressed and resolved. Deferring because a maintainer familiar with the parser's DCE/minify interaction should confirm the invariant that an emptied dead arm plus a NoSideEffects test is sufficient to treat the if as a pure redirect wrapper — I believe it is, but this is exactly the kind of subtle bundler reasoning that benefits from a second pair of eyes.
There was a problem hiding this comment.
No issues found. Deferring only because this changes bundler output for the react NODE_ENV redirect shape and required updating the jsx-production/jsx-dev fixtures — worth a quick maintainer glance at those fixture edits.
What was reviewed:
- Unwrap loop is gated on
SideEffects::to_boolean(returnsok=falsewhen DCE is off) and an already-emptied dead arm, so it only reaches the same post-DCE state--minify-syntaxwould have flattened; downstreammodule_refuse-count and import-record matching are unchanged. cjs2esm: truein the new tests exercises the intended harness path (single outfile, no minify) and asserts zero__commonJSwrappers; the runtime-condition negative case still lists all three files asunhandled.- jsx fixture edits swap the namespace-name check for the redirect-target filename +
jsxDEV(presence; the stdout assertion injsx-production.test.tsis unchanged, so behavioral coverage is preserved.
Extended reasoning...
Overview
The PR extends the module.exports = require(...) redirect detector in src/js_parser/parse/parse_entry.rs to look through a surviving SIf whose test is a compile-time constant with no side effects and whose dead arm has already been emptied by DCE. This lets files shaped like react's index.js NODE_ENV switch collapse to a redirect without --minify-syntax. Six new itBundled cases cover both NODE_ENV values, braced/unbraced bodies, a numeric __DEV__ define, an else-if chain, and the real react package layout with explicit not.toContain checks on the wrapper. Two jsx transpiler fixtures were updated because the react/jsx-runtime wrapper is now eliminated from their bundle output.
Security risks
None. This is AST pattern matching in the parser with no untrusted-input parsing, no allocations, and no FFI.
Level of scrutiny
Medium. The parser change is ~40 LOC, purely additive (a peel loop before the existing match), and provably equivalent to the state --minify-syntax already produces — the same SideEffects::to_boolean predicate that the s_if visitor uses to pick the dead arm gates the unwrap here, and it short-circuits when dead_code_elimination is off. The use_count_estimate == 1 and import-record checks that make the redirect sound are unchanged and run against the same post-DCE symbol/import state either way. The pre-existing ModuleExportsEqualsRuntimeCondition test (non-constant test, all three files stay wrapped) still holds. That said, this changes emitted output for the single most-bundled npm package, and it required editing two existing test fixtures.
Other factors
- The jsx fixture updates replace a namespace-member check (
jsx_dev_runtime.jsxDEV) with a target-file check (jsx-dev-runtime.development) plus ajsxDEV(presence/absence check. The bundled code is still executed viaimport(url)and the parent test still asserts exact stdout, so the property under test is preserved and arguably tightened (prod now also assertsjsxDEV(is absent). Per REVIEW.md this is exactly the case where a reviewer should confirm the edit keeps asserting the original property — I believe it does, but a maintainer familiar with #3768 should confirm. - All bot feedback (comment-cop, coderabbit) is resolved; the four-line comment in
parse_entry.rsdocuments the invariant tying the unwrap to the DCE predicate, matching the surrounding block's existing 20-line NODE_ENV comment. - The
cjs2esm: trueharness path defaultsoutfileviauseOutFile = entryPoints.length === 1, so the harness guard at expectBundled.ts:685 does not fire for the new tests.
28ce7bb to
e90649f
Compare
There was a problem hiding this comment.
I didn't find any issues, but this changes bundler output shape for the ubiquitous NODE_ENV redirect pattern (react, react-dom, etc.) and required updating two existing jsx-runtime test fixtures to match, so it's worth a human look — especially since CI test lanes haven't run yet due to the build-cpp agent expiry.
What was reviewed
- The
SIfpeel loop uses the sameSideEffects::to_booleanpredicate as thes_ifvisitor (visit_stmt.rs:1699) and bails on!ok, side-effecting tests, non-empty dead arms, or multi-statement live blocks — the unwrappedstmtis a local copy so a non-match falls through unchanged. - The unchanged
use_count_estimate == 1guard onmodule_refstill gates the redirect, so a straymodulereference in the dead arm that survived DCE would already have blocked it. cjs2esm: truewithoutminifySyntaxin the new tests is fine — expectBundled.ts:676 defaultsoutfilefor single-entry cases, so the guard at :685 doesn't trip.- The jsx fixture updates keep asserting the same property (correct runtime file bundled + correct jsx call) rather than weakening it.
Extended reasoning...
Overview
Adds a ~40-line while loop in parse_entry.rs that peels constant-test SIf wrappers before the existing module.exports = require(...) redirect match runs. This lets the redirect fire on the standard if (process.env.NODE_ENV === 'production') ... else ... shape without --minify-syntax, dropping the __commonJS wrapper around files like react/index.js. Six new itBundled cases cover both NODE_ENV values, braced/unbraced bodies, a numeric --define, an else-if chain, and the react package path with an onAfterBundle output-shape assertion. Two jsx transpiler fixtures are updated because the jsx_runtime namespace object no longer appears in the output.
Security risks
None. This is a compile-time bundler optimization with no auth, crypto, or untrusted-input parsing surface. The peel is read-only over the already-visited AST and only narrows when a redirect is emitted; on any bail it falls through to the pre-existing behavior.
Level of scrutiny
Medium-high. The logic is small and conservatively guarded, but it changes bundler output for one of the most common package shapes in the npm ecosystem. A subtle mistake here (e.g., dropping a side-effecting test or a hoisted declaration in the dead arm) would produce broken bundles for react users. I checked each guard against the corresponding visitor behavior and they line up, but a maintainer who owns this code path should confirm the interaction with imports_to_convert_from_require / import_records bookkeeping in the react-namespace case (the EIdentifier branch at :1324).
Other factors
- CI: builds #80379/#80888 hit the pipeline-wide
build-cppagent-expiry issue, so no test lanes ran. The author's evidence-gate ASAN run shows fail-before/pass-after for the 6 new cases and 29/29 for the file, plus local verification ofjsx-production.test.tsandbun-serve-html.test.ts, but there's no full-suite CI signal. - All prior review threads (comment-cop, coderabbit) are resolved.
- The jsx fixture rewrite is a legitimate consequence of the output change, not a test weakening — it now asserts on the underlying runtime filename plus the presence/absence of
jsxDEV(, which is at least as strong as the old namespace-name check.
…test if/else
Without --minify-syntax, the NODE_ENV switch in react/index.js survives
as 'if (<const>) {} else { module.exports = X }' after DCE empties the
dead arm. The redirect detector only matched a bare SExpr, so the file
kept its __commonJS wrapper.
Peel the SIf to its live arm's single statement (using the same
SideEffects::to_boolean predicate DCE used to empty the dead arm) before
running the existing module.exports = require() match.
Fixes #12726.
e90649f to
ef2deaf
Compare
There was a problem hiding this comment.
LGTM — targeted extension of the existing module.exports = require() redirect detector to see through a constant-test if, matching what --minify-syntax already produces.
What was reviewed:
- Unwrap loop uses the same
SideEffects::to_booleanpredicate as theSIfvisitor and additionally requiresNoSideEffects+ an already-emptied dead arm, so it cannot fire unless DCE already ran on that branch. - Downstream match is unchanged (still requires
SExpr+use_count_estimate == 1), so unwrapping to anything else falls through harmlessly; existing negative testModuleExportsEqualsRuntimeConditionstill guards the non-constant case. - The jsx-dev / jsx-production fixture updates are required by the output-shape change (react's
jsx-runtimeindex is exactly this pattern) and assert the same property at least as strictly.
Extended reasoning...
Overview
Adds a ~38-line while let SIf unwrap loop in src/js_parser/parse/parse_entry.rs immediately before the existing module.exports = require(...) redirect match. When the file's single non-trivial statement is if (<const>) {} else { <stmt> } (or the reverse), it peels to the live arm's single statement so the existing match can fire. Six new itBundled cases in bundler_cjs2esm.test.ts cover both NODE_ENV polarities, braced/unbraced bodies, a numeric --define, an else-if chain, and the real react package path. Two JSX fixtures update their string-match assertions to track the new (wrapper-free) output shape.
Security risks
None. This is a bundler output-shape optimization; no untrusted input reaches unsafe code, and the transform is semantically identical to what --minify-syntax already emits.
Level of scrutiny
Moderate — parse_entry.rs runs on every bundled file, but the new code is inside the existing parts.len() < 4 && unwrap_commonjs_to_esm guard and only iterates when the single non-trivial statement is an SIf. Every non-matching condition (!effects.ok, side effects, non-empty dead arm, multi-statement live block, SEmpty live arm) breaks out and falls through to the pre-existing behavior. The downstream use_count_estimate == 1 and SExpr-shape checks are untouched, so an over-eager unwrap still cannot produce a redirect unless the resulting statement is exactly module.exports = <ERequireString | namespace-id>.
Other factors
I compared the loop's guards against visit_stmt.rs:1699-1738: the SIf visitor picks the dead arm using only effects.ok, whereas this loop additionally requires side_effects == NoSideEffects, which is strictly more conservative. The dead-arm emptiness check mirrors the SEmpty | SBlock{len==0} pattern the visitor uses to trim else {}. should_keep_stmt_in_dead_control_flow survivors (hoisted var/function) would leave the block non-empty, so the loop breaks. The while handles else-if chains and is exercised by ModuleExportsBasedOnNodeEnvElseIfChainNoMinify. The fixture change in jsx-production.tsx / jsx-dev.tsx swaps the old wrapper-namespace substring for the underlying module filename plus a jsxDEV( presence/absence check — the runtime assertion (<div>Hello World</div> printed twice) in jsx-production.test.ts is unchanged. All prior bot feedback (comment-cop, CodeRabbit) is resolved; no human review comments outstanding.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
The compile break I flagged earlier is fixed in 79f05d6 — the peel loop now uses the Option<Known> shape and matches the other to_boolean callers. I didn't find further issues, but this changes bundler output shape for the react-style NODE_ENV redirect (and required rewriting the jsx-production/jsx-dev fixture assertions to match), so it's worth a human look.
What was reviewed:
- Peel-loop guards:
to_booleanreturnsNonewhen DCE is off, side-effect check bails, dead arm must already be empty, live arm must be exactly one stmt — falls through to old behavior on any miss. whilehandles else-if chains and single-stmt blocks;live == NoneandSEmptyboth break cleanly.use_count_estimate == 1gate is unchanged and already held for the minify-syntax path, so the same DCE bookkeeping applies here.- New
cjs2esm/*NoMinifycases cover both NODE_ENV values, unbraced bodies, numeric define, else-if chain, and the react package path; each has arunstdout assertion.
Extended reasoning...
Overview
Extends the module.exports = require(...) redirect detector in parse_entry.rs to look through a surviving if (<const>) {} else { ... } shell that remains when DCE has emptied the dead arm but --minify-syntax isn't on to flatten the SIf. Adds a ~35-line while peel loop before the existing SExpr match; the match itself and the redirect return are unchanged. Six new itBundled cases in bundler_cjs2esm.test.ts cover the variant matrix, and two JSX transpiler fixtures are updated because react/jsx-runtime now collapses to its target and the bundle no longer contains a jsx_runtime. namespace indirection.
Prior review
My previous inline finding (semantic merge conflict — SideEffects::to_boolean now returns Option<Known>) is resolved by 79f05d6. The new code uses let Some(effects) = ... else { break } and reads effects.side_effects / effects.value off Known, matching visit_stmt.rs and visit_binary.rs.
Security risks
None. This is a pure bundler output-shape optimization gated on DCE having already run; no untrusted-input parsing, no FFI, no allocation changes.
Level of scrutiny
Medium-high. The logic is small and conservatively guarded — every condition that fails to hold breaks back to the pre-existing path — but it changes emitted bundle shape for one of the most common package patterns in the ecosystem (react's NODE_ENV switch), and it required editing two existing test fixtures to keep them passing. The new fixture assertions (jsx-runtime.production filename + absence of jsxDEV() look equivalent in intent to the old ones (jsx_runtime.jsx namespace var), and the runtime stdout checks in those fixtures are unchanged, but a maintainer should confirm they're happy with the assertion swap.
Other factors
The green Buildkite run cited in the PR (#86923) predates the merge with main; #89411 is the build on the current HEAD (79f05d6) and its status wasn't visible to me. The cjs2esm/ModuleExportsEqualsRuntimeCondition test (unknown-at-build-time branch) sits immediately after the new tests and continues to assert the wrapper is kept when the test isn't foldable, which is the negative case for this change.
3 participants
What
Drops the unnecessary
__commonJSwrapper around redirect files likereact/index.jswhen bundling without--minify-syntax.Fixes #12726.
Repro
bun add react@18.3.1 react-dom@18.3.1 bun build input.ts --target=bun | grep -c __commonJSBefore:
5(one is the helper definition, four are wrappers). The fourth wrapper is aroundreact/index.js:After:
4(three wrappers).require_reactis gone and callers userequire_react_developmentdirectly.Cause
react/index.jsis the standard NODE_ENV switch:The define lookup folds the comparison to a boolean literal and DCE empties the dead branch regardless of minification, so after the visitor runs the file's only statement is
if (false) {} else { module.exports = <expr> }. Themodule.exports = require(...)redirect detector inparse_entry.rslooked only for a bareSExpr, so the survivingSIf(which--minify-syntaxwould have flattened) blocked the redirect and the file kept its__commonJSwrapper.Fix
When the single non-trivial statement is an
SIfwhose test is a boolean literal and whose dead branch is empty, unwrap it to the live branch's single statement before running the existingmodule.exports = ...match. The match itself and all the downstream redirect bookkeeping are unchanged.This is guarded by the same conditions that already bound the redirect: the dead branch must have been emptied (which proves
dead_code_eliminationwas on and the branch carried nothing that must survive), and the live branch must contain exactly one statement.Verification
New
cjs2esm/*NoMinifycases intest/bundler/bundler_cjs2esm.test.tscover both NODE_ENV values, braced and unbracedifbodies, and the react package path whererequire()is first rewritten to an ESM namespace identifier. The*NoMinifycases fail on main and pass with this change; the rest ofbundler_cjs2esm.test.tsandbundler_cjs.test.tsare unaffected. Output with--minify-syntaxis byte-identical to before.The
jsx-production.tsx/jsx-dev.tsxfixture updates are required by this change:react/jsx-runtimeships the same NODE_ENV switch, so the redirect now collapses it into its production target and the bundle bindsjsxdirectly instead of going through ajsx_runtimenamespace object. The fixtures asserted the namespace variable name (jsx_runtime.jsx), an artifact of the wrapper this PR removes; they now assert the bundled runtime's file name plus the presence/absence ofjsxDEV(, which holds in both shapes, and the runtime stdout assertions are unchanged.[review] gate passed · iteration 6 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 3 rejected · iteration 6
evidence per changed file