Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 34 seconds Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (6)
Comment |
|
Status: fix pushed (a094395), waiting on CI. Reproduced on the 1.4.0 release with After self-review, |
![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. The gate on is_immediately_assigned_to_decl is small and well-reasoned, but since it changes the bundler's CommonJS-unwrap → declaration-replacement handshake in the visit pass, a human look would still be worthwhile.
What was reviewed:
- Confirmed
visit_declshas only the twos_localcall sites, so thewas_const→kind/is_exportsignature change is complete. - Traced the flag to its sole consumer (
transpose_requireviaTransposeStatein p.rs:1193) — clearing it falls through to the existing namespace-identifier branch, so no marker is left in the tree. - Verified
export import x = require()is parsed asS::Local{is_export, origin: TsImportEquals}, so it reaches this path;LocalKind::is_using()covers bothKUsingandKAwaitUsing. - The
unexportedoutput-shape assertion in the new Converted test pins that plain declarations still take the replace-with-import path.
Extended reasoning...
Overview
The PR changes ~15 production lines across three files in src/js_parser/: the visit_decls signature now takes kind: LocalKind and is_export: bool (instead of the derived was_const), and sets ExprIn.is_immediately_assigned_to_decl only when the declaration is neither exported nor a using/await using. The two call sites in s_local are updated to pass the new arguments, and the field's doc comment is rewritten. Three new itBundled tests in bundler_cjs2esm.test.ts cover export const/var/let, multi-declarator, TS export import = require(), namespace export, a converted-to-ESM target (with a regression pin that non-exported declarations still fold), and using/await using disposal.
Security risks
None. This is a bundler AST-transform correctness fix with no I/O, network, auth, or user-input parsing implications.
Level of scrutiny
Moderate-to-high. The parser's visit pass is a critical bundler code path, and the is_immediately_assigned_to_decl / unwrapped_id handshake between visit_decls and transpose_require is subtle. That said, the change is strictly a narrowing of when the replace-with-import optimization fires — the previous behavior for the affected inputs (export const x = require(unwrapped), using x = require(unwrapped)) was demonstrably broken (dropped export, dropped disposal), and the fallback is the same identifier expression every non-declaration position already produces. I confirmed visit_decls is called only from s_local, so the signature change is complete; was_const is derived identically inside; LocalKind::is_using() covers both using kinds; and the flag's only downstream reader is the !state.is_require_immediately_assigned_to_decl branch in transpose_require, which returns the namespace identifier — exactly what the PR describes.
Other factors
The tests are runtime-asserting (run: { stdout }) rather than snapshot-only, and the Converted test's expect(code).not.toContain("unexported") pins that the plain-declaration fast path is unchanged. The PR description reports the wider bundler suite (bundler_cjs2esm, bundler_npm including ReactSSR's exact-output check, esbuild/default, etc.) passes. The description also flags a one-line rebase interaction with #39184 on the same field. Given the transform's subtlety and the note that #39184 touches the same line, a maintainer sign-off is appropriate even though the change itself looks correct.
cefa344 to
ce03bea
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…pped require()
visit_decls replaces `const x = require("pkg")` with the import the
require() becomes when pkg is on the CommonJS unwrap list, dropping the
declaration. That lost the `export` keyword of exported declarations
(including TypeScript `export import x = require()` and exports inside a
namespace) and the disposal of `using` declarations.
The bool that asked transpose_require for the replaceable-declaration
marker becomes a three-state RequireUnwrap, chosen by visit_decls from
the statement's kind and export flag: exported declarations bind the
import namespace like any other expression and survive, `using` and
`await using` declarations keep an ordinary require() so the value they
dispose is the module's own exports, and everything else folds into the
import as before.
ce03bea to
a094395
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how the cjs→esm require() unwrap is chosen in the parser visit pass — a path that affects React tree-shaking — and introduces a design choice (three-state RequireUnwrap, using > export priority, namespace-binding vs. synthesized export {}) that also intersects with #39184, a human look would still be worthwhile.
What was reviewed:
- Traced
RequireUnwrapthroughExprIn→TransposeState→transpose_require;Namespacedefault is equivalent to the oldfalse, ands_localis the onlyvisit_declscaller. - Checked
is_using()covers bothKUsing/KAwaitUsing, and thatDisabledsurvives throughmaybe_transpose_if_requirefor the conditional-require case. - Confirmed
unwrap_commonjs_to_esmis bundler-only (set inParseTask.rs), so no runtime transpiler cache bump needed. - The three new
itBundledtests cover the exported/converted/using variants and pin the unchanged non-exported path; CI passed.
Extended reasoning...
Overview
This PR fixes a bundler bug where export const X = require("react") (and export var/let, multi-declarator, TS export import =, namespace exports, and using/await using) lose their export or disposal semantics when the required package is on the CommonJS unwrap list. The fix replaces the bool is_immediately_assigned_to_decl with a three-state RequireUnwrap enum (Namespace/IntoDecl/Disabled) threaded through ExprIn, TransposeState, and transpose_require. visit_decls now takes the statement's kind and is_export and picks the mode per-statement. Five parser files touched plus three new bundler tests.
Security risks
None. This is a pure code-transformation change in the bundler's AST visit pass; no user input parsing, no I/O, no auth/crypto.
Level of scrutiny
High. The cjs→esm unwrap path is what enables React tree-shaking in bun build, and a regression here would break real-world React bundles. The change is small in line count but introduces new semantics: a three-valued mode with a priority order (is_using() checked before is_export), and a deliberate choice to bind the import namespace for exports rather than synthesize an export { x } clause. The PR description argues each choice carefully, but these are the kind of design tradeoffs a maintainer should confirm.
Other factors
- Correctness verified locally:
visit_declshas exactly one caller (s_local);TransposeState::default()withRequireUnwrap::Namespaceis semantically identical to the oldis_require_immediately_assigned_to_decl: false;is_using()covers both using kinds; theIntoDecl→Namespacedowngrade for non-string args invisit_expr.rscorrectly preservesDisabledsousing x = require(cond ? a : b)stays un-unwrapped. - No cache bump needed:
unwrap_commonjs_to_esmis only set inbundler/ParseTask.rs, so the runtime transpiler cache is unaffected. - Test coverage: three new
itBundledtests exercise export const/var/let, multi-declarator, TS import-equals, namespace export, converted vs wrapped targets, and using/await-using with actual disposal verification. The existingUnwrappedModuleRequireAssignedtest pins the unchanged non-exportedIntoDeclpath. CI (Build #98831) passed. - Open coordination: the PR notes #39184 touches the same
visit_declssite for destructuring bindings; whichever lands second rebases. - Comment-cop feedback on paragraph-length doc comments was addressed in a094395 and all threads are resolved.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes bundler output for the React unwrap path and the description notes a pending rebase interaction with #39184 at the same visit_decls site, a human look would still be worthwhile.
What was reviewed:
- Confirmed
visit_declshas a single caller (s_local) and the oldis_immediately_assigned_to_decl/is_require_immediately_assigned_to_declfields are fully removed. - Traced
export import x = require()→S::Local { is_export }→RequireUnwrap::Namespace, andDisabledpropagation throughmaybe_transpose_if_requirefor the conditional-usingcase. - Checked
LocalKind::is_using()covers bothKUsingandKAwaitUsing;usingcannot be exported, so its precedence overis_exportin the mode selection is fine.
Extended reasoning...
Overview
This PR fixes export const X = require("react") (and export var/let, multi-declarator, TS export import = require(), namespace exports, and using/await using) dropping the export or disposal when the required package is on the CommonJS unwrap list. The bool is_immediately_assigned_to_decl on ExprIn/TransposeState becomes a three-state RequireUnwrap enum (Namespace/IntoDecl/Disabled); visit_decls now takes kind + is_export and picks the mode once per statement; transpose_require gates should_unwrap_require on != Disabled and the marker return on == IntoDecl. Three new itBundled tests in bundler_cjs2esm.test.ts cover the variant matrix.
Security risks
None. This is bundler AST-rewrite logic with no auth, crypto, filesystem, or network surface. The change is a pure refactor of how a require-unwrap flag is threaded through the visit pass.
Level of scrutiny
Medium-high. The JS parser visit pass and transpose_require are on the bundler hot path and directly shape emitted output for React (the primary consumer of the unwrap list). The mechanical change is small and well-contained — a bool→enum widening threaded through the exact same three sites — but the semantic decision (bind exported declarations to the namespace identifier rather than synthesizing an export { x } after folding) has a stated tree-shaking tradeoff the author measured and argued. That's the kind of shape choice a maintainer should confirm.
Other factors
The PR description is unusually thorough: root cause traced to the unwrapped_id marker consumption in visit_decls, alternatives weighed (synthesized export { x } rejected for extending an existing let-reassignment mishandling to exports; namespace binding for using rejected because __toESM drops symbol-keyed properties), and unchanged paths pinned by named existing tests (UnwrappedModuleRequireAssigned, npm/ReactSSR). The comment-cop feedback was addressed (docs trimmed to one line per variant). I verified visit_decls has only the one s_local caller, the old field names are gone repo-wide, export import x = require() lowers to S::Local { is_export } so it reaches the fix, Disabled propagates through maybe_transpose_if_require for conditional requires, and is_using() covers both KUsing and KAwaitUsing. The explicit note that #39184 touches the same visit_decls site and whichever lands second must rebase is another reason to have a maintainer coordinate the merge order.
This branch has not been deployed
1 participant
Problem
bun build(ESM output, the default) drops the export of a declaration initialized by arequire()of a package on the CommonJS unwrap list (react,react-dom,scheduler, ...). Same on 1.4.0 and main:lib.jsalone printsvar React = __toESM(require_react(), 1);with noexport { React }.export var/export let, the other declarators of a multi-declaratorexport const, TypeScriptexport import React = require("react"),export constinside a TypeScript namespace (theNS.React = ...assignment disappears), and all of these when the required file converts to ESM instead of staying wrapped.using x = require("react")/await usinglose their disposal the same way: the bundle prints a hoistedvarand the value is never disposed.visit_decls(src/js_parser/visit/mod.rs) visits every initializer withExprIn.is_immediately_assigned_to_decl.transpose_require(src/js_parser/p.rs, unwrap branch) answers the flag with anE::RequireStringmarker, andvisit_declsconsumes it by renaming the pendingimport * as nsto the declared name and removing the declarator. With every declarator gone,s_local(src/js_parser/visit/visit_stmt.rs) drops the wholeS::Local, and with it theexportkeyword (scan_importsrecords exports from theS::Localstatements that survive the visit) or theusingkind (nothing is left to lower). For a plainconst x = require("react")that is the intended rewrite; an import statement cannot stand in for an exported orusingdeclaration.Fix
RequireUnwrap(src/js_parser/parser.rs), carried throughExprInandTransposeStateexactly where the bool was.visit_declsnow takes the statement'skindandis_export(it tookwas_const, now derived fromkind) and picks the mode once per statement:Namespace.transpose_requiretakes its existing other branch and returns the import namespace identifier, which is what arequire()of an unwrapped package already becomes in every non-declaration position. The declaration survives with that initializer, so the export is recorded. Exports inside a namespace need nothing extra:is_exportis set there too, ands_local's namespace branch emits theNS.x = ...assignment once the declaration survives.using/await using:Disabled.transpose_requireANDs it intoshould_unwrap_requirenext to the existing try/catch condition, so therequire()stays an ordinary require and the declaration binds the module's own exports value. It flows throughrequire(a ? "x" : "y")as well, sincemaybe_transpose_if_requirepasses the same state to both branches.IntoDecl, the previous behavior.visit_exprstill downgrades it toNamespacewhen the argument is not a string literal (the old&& first is EString).visit_declscan replace the declaration, so the decision belongs where the marker is produced. Gating the consumer instead would leave the marker in the tree for the printer, whose fallback for a surviving marker is the path bundler: read destructured require() of an unwrapped package from its import namespace #39184 is fixing.var react = __toESM(require_react(), 1); var React = react;, converted givesvar React = exports_react;. Keeping the fold and synthesizing anexport { x }instead would save the alias and, when the export is unused by other files, let the file's ownx.fooreads tree-shake the package (measured: 3 exports retained vs 1). That case was only "optimal" before because the export was silently missing; when the export is consumed, both shapes materialize the namespace object (measured identical apart from the alias line); and foldingexport let x = require(..)would extend the existing mishandling of laterx = ...assignments to exports. Not worth a second code path in this fix.using, binding the namespace is not enough:__toESM()copies only string-keyed own properties, as getters, and a converted module's namespace object has no symbols at all, somodule.exports = { [Symbol.dispose]() {} }orexports[Symbol.asyncDispose] = ...(the shapes a disposable CommonJS module actually has; both stay wrapped) would throwTypeError: Object not disposablein the bundle while working unbundled. Treating the site like a try/catch is the existing mechanism for "this require() must stay a require()", and the bundle then prints exactly what the source does unbundled (verified: the new test's expected stdout is the unbundled output of its entry).unexportedassertion in the new converted test and by the existingcjs2esm/UnwrappedModuleRequireAssignedandnpm/ReactSSRtests (the React packages have noexportkeywords, and theirexports.x = require(...)statements are rewritten ins_exprafter the expression visit, so they never reach this path;npm/ReactSSR's exact output is unchanged).test/bundler/bundler_cjs2esm.test.ts; the three new tests fail on the current release and pass with this change:cjs2esm/UnwrappedModuleRequireExported:export const/var/let, a multi-declaratorexport const,export import x = require()and an export inside a namespace, imported and read from an entry (before:No matching exportfor each).cjs2esm/UnwrappedModuleRequireExportedConverted:export constagainst a package that converts to ESM printsvar exported = exports_react;; a non-exported declaration in the same file is still folded into the import.cjs2esm/UnwrappedModuleRequireUsing:using,await usingand ausingof a conditionalrequire()against modules with ownSymbol.dispose/Symbol.asyncDisposehooks: the bound values are identical to the modules' exports (===against animportof the same packages), the hooks run at block exit with the module asthis, and the output equals the unbundled run (before:TypeError: Object not disposable).bundler_cjs2esm,bundler_cjs,bundler_edgecase,bundler_regressions,bundler_npm,bundler_bun,bundler_minify,esbuild/defaultandtranspiler/transpiler.test.jswith the debug build: no failures.IntoDeclonly forB::Identifierbindings. Whichever lands second rebases the one site invisit_declsplus theExprIncomment.Background
DEFAULT_UNWRAP_COMMONJS_PACKAGES(src/bundler/options.rs) are parsed withexports.x = ...turned into ESM exports, and everyrequire()resolving into one of those packages, from any file, becomes animport * as ns from "..."statement (emitted at the end of the parse fromimports_to_convert_from_require) plus a reference tons. This is what lets React tree-shake. Arequire()inside a try/catch is the existing exception: it stays a plain require because an import cannot be caught.module.exports(or anything else the converter cannot express, such as a symbol-keyedexports[...] = ...) stays CommonJS and is emitted asvar require_x = __commonJS(...); a plainrequire()of it printsrequire_x()and its import namespace prints__toESM(require_x(), 1). A file that only usesexports.x = ...converts, and its namespace prints as the linker's generated namespace object (exports_x).E::RequireString.unwrapped_id: the markertranspose_requirereturns inIntoDeclmode; it indexes the pending import, andvisit_declsuses it to rename that import's namespace to the declared binding and drop the declarator, turningconst React = require("react")intoimport * as React from "react". Nothing else produces or reads it.[review] gate passed · iteration 0 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file