Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes 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: ready for review. Latest push (4f39150) adds a debug assert pinning the invariant behind the deletion and positive output assertions in the new test, after a second review pass; waiting on CI for that push. Reproduced on bun 1.4.0 and on a debug build of main with The second commit deletes the printer/linker |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted one-line fix at the right layer, with thorough tests.
What was reviewed:
- Traced
is_immediately_assigned_to_declend-to-end: sole setter invisit_decls, sole reader invisit_expr.rs:2022→transpose_require(p.rs:1193); the marker it produces is only consumed by theBIdentifierbranch that follows, so gating the flag on that binding shape is exactly right. - Checked the mixed-declarator case (
const before = require(...), { between } = require(...), after = require(...)) — the flag is computed per-declarator, so identifier bindings still take the rename-and-drop path. - Tests cover object/array/nested/defaulted destructuring, mixed declarators, function-body scope, and pin the converted-target path so both wrapped and unwrapped targets are exercised.
Extended reasoning...
Overview
The PR fixes a bundler bug where const { x } = require("react") (or any destructuring binding) of a package on the CommonJS unwrap list produced (__toESM(exports, 1)) — a reference to the wrapped module's callback parameter, unbound at the use site — instead of the namespace identifier the generated import * as ns binds. The behavioral change is one line in src/js_parser/visit/mod.rs: is_immediately_assigned_to_decl is now set only when the binding is BIdentifier. A doc comment on the ExprIn field is tightened to match, and two itBundled tests are added.
Security risks
None. This is a bundler code-transformation change with no auth, crypto, filesystem, or network surface.
Level of scrutiny
Medium. The change is a single boolean condition in a well-understood parser path, and the PR description demonstrates a complete trace of the mechanism (marker producer in transpose_require, marker consumer in visit_decls, printer fallback in LinkerContext). I confirmed via grep that the flag has exactly one setter and one consumer chain, so restricting the setter to the shape the consumer handles is provably the whole class. The identifier-binding path (const React = require("react") → rename the import namespace, drop the decl) is unchanged because matches!(.., BIdentifier(_)) is true there.
Other factors
- Test coverage is strong: the new
UnwrappedModuleRequireDestructuredtest covers six destructuring shapes and asserts both output (not.toMatch(/__toESM\(exports\b/)) and runtime stdout;UnwrappedModuleRequireDestructuredConvertedpins the already-working converted-target path to the new code path it now shares. The mixed-declarator case (before, {between}, afterin oneconst) verifies per-declarator flag evaluation. - The author reports the broader bundler suites (
bundler_cjs2esm,bundler_cjs,bundler_edgecase,bundler_regressions,bundler_jsx,bundler_npmincluding theReactSSRbyte-size check,esbuild/default, transpiler tests) all pass unchanged. - No prior human or bot reviews to address; CI is building.
4e8d3a6 to
6a0d169
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
6a0d169 to
a690bbe
Compare
There was a problem hiding this comment.
I re-reviewed after a690bbe — the two comment-length nits from my earlier pass are addressed and the bug-hunting system found nothing this run. Given this changes bundler output on the React unwrap path and removes the was_unwrapped_require printer/linker fallback, a maintainer look is still worthwhile.
What was reviewed:
- Traced
is_immediately_assigned_to_decl→transpose_require→visit_declsconsumer: with the newBIdentifiergate, everyERequireStringcarryingunwrapped_idis consumed by the identifier-rename branch (should_unwrap_common_js_to_esm()and theprev_require_to_convert_countcheck both hold whenevershould_unwrap_requirefired), so none reaches the printer. - Checked the removed
FORCE_CJS_TO_ESM/was_unwrapped_requirebranches inLinkerContext::require_or_import_meta_for_sourceandprint_require_or_import_expr— all callers now pass the equivalent offalse, so the deletions are behavior-preserving. - Confirmed the
module.exports = require(...)reader atparse_entry.rs:1475is unaffected (that path never hadis_immediately_assigned_to_declset).
Extended reasoning...
Overview
The PR fixes destructured require() of a package on the CommonJS unwrap list (react, react-dom, scheduler, …) when the target module stays wrapped. The core fix is one line in src/js_parser/visit/mod.rs: is_immediately_assigned_to_decl is now only set when the declaration binding is a plain identifier, so transpose_require returns the import-namespace identifier (rather than the ERequireString marker) for object/array destructuring. A second commit removes the printer/linker was_unwrapped_require path that this change makes unreachable — the callback signature in RequireOrImportMetaSource/RequireOrImportMetaCallback, the RequireOrImportMeta.was_unwrapped_require field, the FORCE_CJS_TO_ESM special case in LinkerContext::require_or_import_meta_for_source, and the meta.exports_ref printing branch. A one-line doc comment on ExprIn::is_immediately_assigned_to_decl and a new cjs2esm/UnwrappedModuleRequireDestructured bundler test (plus expanded coverage in the existing ...AndInTry test) round it out.
Security risks
None. This is bundler codegen; no untrusted input parsing, auth, crypto, or filesystem write logic is touched.
Level of scrutiny
Moderate-to-high. The core parser change is tiny and clearly correct, but the second commit deletes a fallback path across the printer↔linker callback boundary on the strength of an invariant ("no unwrapped_id reaches the printer any more"). I traced it: unwrapped_id is only set at p.rs:1203 when state.is_require_immediately_assigned_to_decl is true, which after this PR is only true for a top-level require("literal") initializer of a BIdentifier decl; that exact shape is always consumed by the continue 'outer branch in visit_decls (the two extra gates there — should_unwrap_common_js_to_esm() and prev_require_to_convert_count < len — are both implied by should_unwrap_require having fired). in_.is_immediately_assigned_to_decl does not propagate into nested sub-expression visits (p.visit_expr(arg) uses ExprIn::default()), so a nested require() cannot get the marker. The other unwrapped_id reader at parse_entry.rs:1475 is unaffected by this PR. Still, this is the React-unwrap path — the single most common bundling scenario — and the deletion touches three crates, so a maintainer confirming the invariant is worth the extra look.
Other factors
The author addressed my earlier comment-cop note in a690bbe (both new comments are one line each). Test coverage is thorough: object/renamed/defaulted/nested/array destructuring, a destructuring declarator mixed with identifier declarators in one statement, one inside a function body, and one against a package that does convert to ESM; the existing UnwrappedModuleRequireAssigned test already covers the non-declaration expression positions this fix aligns with. The PR description reports passes across bundler_cjs2esm, bundler_cjs, bundler_edgecase, bundler_regressions, bundler_jsx, bundler_splitting, bundler_npm (ReactSSR byte size unchanged), and the esbuild suites.
…ort namespace When a require() of a package in the CommonJS unwrap list (react, react-dom, ...) initializes a declaration, the parser returns an E::RequireString marker so visit_decls can rename the generated import namespace to the declared identifier and drop the declaration. Only identifier bindings are handled there; for a destructuring binding the marker survived to the printer, which printed the target module's own exports ref. When that module could not be converted to ESM (it assigns module.exports), that ref is the __commonJS wrapper's local `exports` parameter, so the destructuring read from an unrelated `exports` binding and every property came out undefined. Only set is_immediately_assigned_to_decl for identifier bindings, so a destructuring initializer gets the namespace identifier that require() becomes in every other expression position.
With the parser only producing the unwrapped RequireString marker for identifier bindings, which visit_decls always removes, no RequireString with unwrapped_id set reaches the printer any more. Remove the flag the printer threaded through RequireOrImportMeta and its callback, the printer branch that printed the target's exports_ref for it, and the FORCE_CJS_TO_ESM special case in LinkerContext::require_or_import_meta_for_source. The printer now debug-asserts that invariant where it used to read the marker.
a690bbe to
4f39150
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs; the earlier comment-length nit was addressed in a690bbe. Because the second commit removes the was_unwrapped_require printing path across the parser/printer/linker boundary based on the invariant that no E::RequireString with unwrapped_id set reaches the printer, a human look at that invariant would still be worthwhile.
What was reviewed:
- Traced
is_immediately_assigned_to_decl→transpose_require→visit_declsconsumer: with theBIdentifiergate, the marker is produced exactly when the consumer handles it (continue 'outer). - Checked that the removed
FORCE_CJS_TO_ESMbranch inrequire_or_import_meta_for_sourceand the printer'smeta.exports_reffallback have no remaining producer; the newdebug_assert!(e.unwrapped_id.is_none())guards this in debug builds. - Confirmed the
parse_entry.rsreader ofunwrapped_id(themodule.exports = require(...)→S::ExportStarrewrite) is unaffected — it's an expression-statement path whereis_immediately_assigned_to_declwas already false. - New test covers object/array/nested/renamed/defaulted destructuring, mixed declarators, and function-body scope against both wrapped and converted targets.
Extended reasoning...
Overview
The PR fixes bundler output for const { x } = require("react") when react is on the CommonJS-unwrap list but the target module stays wrapped (assigns module.exports). The functional fix is one line in src/js_parser/visit/mod.rs:323 — is_immediately_assigned_to_decl is now only set for identifier bindings, so destructuring initializers get the namespace identifier instead of the E::RequireString marker. The second commit deletes the now-unreachable was_unwrapped_require plumbing across js_printer/lib.rs and bundler/LinkerContext.rs (~55 net lines removed), plus doc-comment updates in parser.rs and ast/e.rs, and adds a comprehensive test in bundler_cjs2esm.test.ts.
Security risks
None. This is bundler code-generation logic with no user-input validation, network, filesystem, or crypto surface. The change narrows when a marker is produced and removes a printing path; no new attack surface.
Level of scrutiny
High — bundler output correctness is production-critical (miscompiled require() of react affects real apps), and the dead-code deletion spans a parser→printer→linker invariant. The one-line fix itself is straightforward and well-scoped; the cross-crate deletion is what warrants a maintainer's confirmation. I traced the invariant end-to-end (transpose_require in p.rs:1193 sets unwrapped_id only when is_require_immediately_assigned_to_decl, which after this PR is only true for BIdentifier bindings, which the visit_decls consumer at mod.rs:359-372 always handles via continue 'outer), and it holds for the direct case. The new debug_assert in the printer would catch any violation in CI.
Other factors
- CI green on a690bbe (Buildkite #98803) and the description reports the full bundler suite (
bundler_cjs2esm,bundler_cjs,bundler_edgecase,bundler_regressions,bundler_jsx,bundler_splitting,bundler_npmincluding the byte-exactnpm/ReactSSR,esbuild/{default,splitting,importstar}, transpiler tests) passes unchanged. - The new
UnwrappedModuleRequireDestructuredtest covers the variant matrix per REVIEW.md (object/array/nested/renamed/defaulted destructuring, mixed declarators in one statement, function-body scope, both wrapped and converted targets) and asserts both output shape and runtime behavior. - My prior review (comment-cop style nit) was addressed; both flagged comments are now one line and the threads are resolved.
- The PR description's root-cause analysis is unusually thorough and matches what I found in the code.
…JS at link time (#41237) ### Problem - With `--splitting`, a cross-chunk `import()` of a module that is CommonJS at link time resolves to the bare chunk namespace `{ default: module.exports }`. With npm react-dom 18.3.1, `(await import("react-dom/client")).createRoot` is `undefined`. Unsplit builds give the function. - Cause: the skip at `src/bundler/linker_context/scanImportsAndExports.rs:1116` drops the `__toESM` wrap for every `import()` target with `FORCE_CJS_TO_ESM`. Such a target can still be CommonJS at link time, with the chunk `export default require_x()`. ### Fix - Remove the skip. The existing cross-chunk branch then adds `.then((m) => __toESM(m.default))` for a CommonJS target, and nothing for an ESM target. - Correct because splitting is ESM output only, where `exports_kind == Cjs` means the chunk exports only `default: module.exports`. Since #41231 the skip did nothing else, so ESM targets print the same. - Verified: three new cases in `test/bundler/bundler_cjs2esm.test.ts` fail on 1.4.1 and on main, and pass with this change. - Self-reviewed: 3 concerns raised, 3 addressed. Most of the diff is re-indentation. Hide whitespace to see the change. ### Background - Lifting: in an ESM bundle, the parser turns top-level `exports.foo = ...` into ES module exports and sets `FORCE_CJS_TO_ESM`. Every file of the unwrap list (react, react-dom, ...) gets the flag, lifted or not. - A flagged file is CommonJS at link time when it assigns `module.exports`, when a `require()` of it wraps it, or when the target of its lifted `module.exports = require()` is CommonJS (#41188). - With code splitting, each `import()` target gets its own entry point chunk. <details><summary>Notes</summary> No issue reports this. It was found during work on the nearby CommonJS lifting code. An earlier version of this PR narrowed the skip to `exports_kind != Cjs`. After the rebase on #41231, the body of the skip was only `continue`, so the narrowed skip did nothing. This version removes it. The self-review found a wrong comment about `FORCE_CJS_TO_ESM` (now removed with the code). It also asked for a test outside the unwrap list and for a fuller description. Real packages (react 18.3.1, react-dom 18.3.1, scheduler 0.23.2), entry: ```js const { createRoot } = await import("react-dom/client"); const React = await import("react"); const Scheduler = await import("scheduler"); console.log(typeof createRoot, typeof React.useState, typeof Scheduler.unstable_scheduleCallback); ``` | build | 1.4.1 | this branch | | --- | --- | --- | | `--splitting`, browser or bun target, development or production | `undefined undefined undefined` | `function function function` | | `--splitting --minify`, production | `undefined function undefined` | `function function function` | | `--splitting --minify`, development | `undefined undefined undefined` | `function function function` | | no `--splitting` | `function function function` | `function function function` | `bun run` of the entry prints `function function function`. Minimal repro in an empty directory. Same output on 1.4.1 and main: ```sh mkdir -p node_modules/react/cjs echo '{ "name": "react", "version": "19.0.0", "main": "index.js" }' > node_modules/react/package.json printf "'use strict';\nif (process.env.NODE_ENV === 'production') {\n module.exports = require('./cjs/react.production.js');\n} else {\n module.exports = require('./cjs/react.development.js');\n}\n" > node_modules/react/index.js printf "'use strict';\nfunction useState(i) { return [i, function () {}]; }\nexports.useState = useState;\nexports.version = '19.0.0';\n" > node_modules/react/cjs/react.production.js cp node_modules/react/cjs/react.production.js node_modules/react/cjs/react.development.js printf 'import React from "react";\nconst m = await import("react");\nconsole.log(m.useState(1)[0], m.default === React);\n' > entry.mjs NODE_ENV=production bun build ./entry.mjs --splitting --target=bun --outdir=out && bun out/entry.js ``` Before: `TypeError: m.useState is not a function`. After: `1 true`. The importer now prints `await import("./index-<hash>.js").then((m)=>__toESM(m.default,1))`. The three new cases, one for each way to be CommonJS at link time: - `cjs2esm/DynamicImportSplittingOfWrappedCommonJS`: `react/index.js` is a run-time `if` over two `module.exports = require()` calls, so it is never lifted. - `cjs2esm/DynamicImportSplittingOfRewrappedLiftedCommonJS`: the `ReactSpecificUnwrappingTargetIsCommonJS` fixture from #41188. The linker wraps `react-dom/index.js` again because `impl.js` assigns `module.exports = function`. It prints `m.default.version`, not `typeof m.default`, so it does not depend on #35722. - `cjs2esm/DynamicImportSplittingOfRequiredLiftedCommonJS`: a user file with `exports.foo = ...`, outside `node_modules`. The entry `require()`s it and `import()`s it. Before: `foo undefined true`. The unsplit build and `bun run` print `foo foo true`. The `require()` side of the same shape was a regression from #41188 (#41236). #41243 fixed it on main, in the same block. This PR changes only `import()` records. Not changed (each reproduces with and without this change): - `const { useState } = require("react")` throws `ReferenceError: exports is not defined` in a bundle, split or not. #39184 fixes it. - With `--splitting`, `import()` of a file that does `export * from "<cjs>"` reads `undefined` for the names of the CommonJS module. This happens for any CommonJS package. - #41231 (merged) made the chunk of a lifted target export its namespace as `default`. It keeps the skip for a CommonJS target, so it does not cover this bug. This branch is rebased on it, and its tests pass here. The unwrap list is `DEFAULT_UNWRAP_COMMONJS_PACKAGES` in `src/bundler/options.rs`: react, react-dom, scheduler, react-is, react-refresh, react-client, react-server. Also checked with the debug build under `--splitting`: `module.exports = { ... }`, `module.exports = function`, an importer whose only `__toESM` use is the `import()` (it gets the runtime import), and a `.js` importer (`__toESM(m.default)` without the node-mode flag, as on the existing path). A user file that is only `import()`ed, and a real `module.exports` file, print the same before and after. Suites run with the debug build on main 1d1f431 (after #41231 and #41243), with this version of the fix: bundler_cjs2esm and bundler_splitting (188 pass), and bundler_cjs, bundler_dynamic_import_dce, esbuild/splitting (358 pass, 0 fail). Before those rebases, on main e8c8d81: the same suites plus esbuild/default, bundler_edgecase, bundler_regressions, bundler_npm, bundler_compile_splitting, bundler_bun, bundler_browser (904 pass, 0 fail). `cargo clippy -p bun_bundler` is clean. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 3 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/bundler/bundler_cjs2esm.test.ts" bun test v1.4.1 (a6c4cc2) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [808.95ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [428.20ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [378.09ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [467.65ms] (pass) bundler > cjs2esm/ExportsFunction [371.77ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [454.02ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [429.96ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [371.66ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [479.66ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [494.23ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [407.11ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [706.09ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [575 ... (truncated) release without fix: all passed bun test v1.4.1-canary.1 (b36f032) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [19.49ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [9.57ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [8.62ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [7.87ms] (pass) bundler > cjs2esm/ExportsFunction [8.03ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [8.04ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [7.78ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [8.72ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [9.61ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [9.77ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [9.36ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [11.65ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [11.04ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRuntimeCondition [10.16ms] (pass) bundler > cjs2esm/UnwrappedModuleRequireAssigned [9.16ms] (pass) bundler > cjs2esm/UnwrappedModuleRe ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/bundler/bundler_cjs2esm.test.ts" bun test v1.4.1 (a6c4cc2) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [835.93ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [485.38ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [375.10ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [342.95ms] (pass) bundler > cjs2esm/ExportsFunction [429.44ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [433.97ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [347.36ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [436.35ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [424.28ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [513.08ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [375.98ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [674.86ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [643 ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 635ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/1] reconfigure [1/10] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 244 extern-C blocks audited [2/10] gen cpp.rs (cppbind) [2/10] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_brotli v0.0.0 (/wor ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` .../linker_context/scanImportsAndExports.rs | 134 ++++++++++----------- test/bundler/bundler_cjs2esm.test.ts | 95 +++++++++++++++ 2 files changed, 157 insertions(+), 72 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/bundler/linker_context/scanImportsAndExports.rs 8 4 41 test/bundler/bundler_cjs2esm.test.ts 4 3 40 ``` </details> <!-- robobun:evidence:end -->
|
#42500 is stacked on this PR. Its first two commits are this branch on top of current main, and a third commit removes two readers of the marker that main gained since ( The fix in #42500 (an ES module installed as |
This branch has not been deployed
1 participant
Problem
bun build(ESM output, the default) of an entry that destructures arequire()of a package in the CommonJS unwrap list readsundefinedfor every property when that package's module stays CommonJS (it assignsmodule.exports):TypeError: {} is not iterableinstead. Same output on 1.4.0 and on main.reactinstall:react/index.jsis amodule.exports = require("./cjs/...")redirect that stays wrapped, soconst { useState } = require("react")in a bundled entry givesuseState === undefined(verified with a copy of that layout).transpose_require(src/js_parser/p.rs:1193) returns anE::RequireStringmarker instead of the namespace identifier whenever therequire()is a declaration initializer.visit_decls(src/js_parser/visit/mod.rs:361) only consumes the marker for an identifier binding; for a destructuring binding it survived to the printer. The printer's path for a surviving marker (print_require_or_import_exprwithwas_unwrapped_require, viaLinkerContext::require_or_import_meta_for_source) printed the target'sexports_refwhenever the target hasFORCE_CJS_TO_ESM. That flag is set at parse time for every file in an unwrapped package (src/js_parser/p.rs:8449) whether or not the file converted, and for a file that did not convertexports_refis the__commonJScallback'sexportsparameter.Fix
visit_declssetsis_immediately_assigned_to_declonly when the binding is an identifier, so a destructuring initializer gets the same namespace identifier thatrequire()of an unwrapped package becomes in every other expression position (call argument, assignment right-hand side,require(...).x, ...). The case above now bundles tovar { react: react2 } = react;.visit_decls(rename the generatedimport * as nsto the declared name, drop the declaration). Producing it for a binding that rewrite does not handle was the defect; now it is produced exactly when it is consumed.cjs2esm/UnwrappedModuleRequireAssignedtest already exercises for the non-declaration positions, and it is right for both target shapes: a wrapped target prints it as__toESM(require_x(), 1), a converted target prints it as the generated namespace object (exports_react), which is what the removed path printed for a converted target.RequireStringwithunwrapped_idset reaches the printer any more (second commit, no output change): thewas_unwrapped_requireargument ofprint_require_or_import_exprand of therequire_or_import_meta_for_sourcecallback (RequireOrImportMetaCallback,RequireOrImportMetaSource,Options::require_or_import_meta_for_source), theRequireOrImportMeta.was_unwrapped_requirefield, the printer branch that printedmeta.exports_reffor it, and theFORCE_CJS_TO_ESMspecial case inLinkerContext::require_or_import_meta_for_source, which now returnsexports_refonly forwrap == Esm.unwrapped_iditself stays (visit_declsreads it); the printer'sERequireStringarm nowdebug_asserts that it is unset, so the whole bundler suite under a debug build checks the invariant the deletion relies on.test/bundler/bundler_cjs2esm.test.ts:cjs2esm/UnwrappedModuleRequireDestructured(new): object, renamed and defaulted, nested and array destructuring, a destructuring declarator between two identifier declarators in one statement, and one inside a function body, all against packages that stay wrapped. It asserts both packages are still emitted as__commonJS(wrappers and that the destructurings read from an identifier, then runs the bundle. Fails on main on every form (each prints(__toESM(exports, 1))), passes with this change.cjs2esm/UnwrappedModuleRequireDestructuredAndInTry(from js_parser: type RequireString.unwrapped_id as an optional index #39169): its comment described the removed marker path, so it is reworded, and it now also covers a defaulted property and an identifier declarator next to a destructuring one, against a package that does convert; the try/catchrequire()it already had covers the ordinaryRequireStringprinting that remains. It passes before and after this change.bundler_cjs2esm,bundler_cjs,bundler_edgecase,bundler_regressions,bundler_jsx,bundler_splitting,bundler_npm(thenpm/ReactSSRexact file size is unchanged),esbuild/default,esbuild/splitting,esbuild/importstar, andtranspiler/transpiler.test.jswith the debug build: no failures.cargo clippyonbun_js_printerandbun_bundleris clean.export const X = require("react")loses its export through the same drop-the-declaration rewrite; that is a separate defect with its own repro and is tracked separately.Background
react,react-dom,scheduler,react-is,react-refresh,react-clientandreact-server(DEFAULT_UNWRAP_COMMONJS_PACKAGESinsrc/bundler/options.rs) are parsed withexports.x = ...turned into ESM exports, and everyrequire()that resolves into one of those packages (from any file) is turned into animport * as ns from "..."statement plus a reference tons, so the package tree-shakes. The import statements are emitted from the parser'simports_to_convert_from_requirelist at the end of the parse.exports.x = ...becomes ESM and the linker gives it a namespace object (var exports_react = {}; __export(exports_react, {...})), which is itsexports_ref. A file that assignsmodule.exportscannot be converted and is emitted asvar require_react = __commonJS(function(exports, module) {...}); itsexports_refis that callback'sexportsparameter, which only exists inside the callback.FORCE_CJS_TO_ESMis set on both kinds of file, so it does not tell the two apart.is_immediately_assigned_to_decl/E::RequireString.unwrapped_id:visit_declspasses this flag when visiting a declaration's initializer;transpose_requireanswers it by returning anE::RequireStringwhoseunwrapped_idindexes the pending import, andvisit_declsuses that index to rename the import's namespace to the declared identifier and remove the declaration (const React = require("react")becomesimport * as React from "react"). This flag has no other reader.RequireOrImportMeta: what the printer asks the linker for when printing a bundledrequire()/import()of another file: the file's wrapper function (require_xfor CommonJS,init_xfor lazily initialized ESM) and, for wrapped ESM, its namespace object, printed as(init_x(), __toCommonJS(exports_x)).[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