Repository navigation
Reduce the number of closures in generated bundler code - #27022
Conversation
|
Updated 1:16 AM PT - Feb 15th, 2026
❌ @Jarred-Sumner, your commit 14e0298 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 27022That installs a local version of the PR into your bun-27022 --bun |
WalkthroughRefactors runtime internals: adds shared Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime.js (1)
48-78:⚠️ Potential issue | 🔴 CriticalGuard WeakMap caching against primitive and non-object exports.
WeakMaponly accepts object/function keys. If a CJS module exports a primitive (e.g.,module.exports = 42),cache.set()throwsTypeError. Add type guards to skip caching for non-objects. Note:__toCommonJS(line 86–97) has the identical issue.🩹 Proposed fix for __toESM
- if (mod != null) { - var cache = isNodeMode ? (__toESMCache_node ??= new WeakMap()) : (__toESMCache_esm ??= new WeakMap()); - var cached = cache.get(mod); - if (cached) return cached; - } + var cache; + if (mod && (typeof mod === "object" || typeof mod === "function")) { + cache = isNodeMode ? (__toESMCache_node ??= new WeakMap()) : (__toESMCache_esm ??= new WeakMap()); + var cached = cache.get(mod); + if (cached) return cached; + } target = mod != null ? __create(__getProtoOf(mod)) : {}; const to = isNodeMode || !mod || !mod.__esModule ? __defProp(target, "default", { value: mod, enumerable: true }) : target; @@ - if (mod != null) cache.set(mod, to); + if (cache) cache.set(mod, to); return to;
Code ReviewNewest first ✅ b4543 — Looks good! Reviewed 7 files across ✅ 7f12f — Looks good! Reviewed 1 file in Powered by Claude Code Review |
Code ReviewNewest first ✅ 93218 — Looks good! Reviewed 7 files across Powered by Claude Code Review |
Code ReviewNewest first ✅ 3472a — Looks good! Reviewed 7 files across ✅ b4543 — Looks good! Reviewed 7 files across ✅ 7f12f — Looks good! Reviewed 1 file in Powered by Claude Code Review |
### Problem The bundler's `__toESM` helper creates a new getter-wrapped proxy object every time a CJS module is imported. In a large app, a popular dependency like React can be imported 600+ times — each creating a fresh object with ~44 getter properties. This produces ~27K unnecessary `GetterSetter` objects, ~25K closures, and ~25K `JSLexicalEnvironment` scope objects at startup. Additionally, `__export` and `__exportValue` use `var`-scoped loop variables captured by setter closures, meaning all setters incorrectly reference the last iterated key (a latent bug). ### Changes 1. **`__toESM`: add WeakMap cache** — deduplicate repeated wrappings of the same CJS module. Two caches (one per `isNodeMode` value) to handle both import modes correctly. 2. **Replace closures with `.bind()`** — `() => obj[key]` becomes `__accessProp.bind(obj, key)`. BoundFunction is cheaper than Function + JSLexicalEnvironment, and frees the for-in `JSPropertyNameEnumerator` from the closure scope. 3. **Fix var-scoping bug in `__export`/`__exportValue`** — setter closures captured a shared `var name` and would all modify the last iterated key. `.bind()` eagerly captures the correct key per iteration. 4. **`__toCommonJS`: `.map()` → `for..of`** — eliminates throwaway array allocation. 5. **`__reExport`: single `getOwnPropertyNames` call** — was calling it twice when `secondTarget` was provided. ### Impact (measured on a ~23MB single-bundle app with 600+ React imports) | Metric | Before | After | Delta | |--------|--------|-------|-------| | **Total objects** | 745,985 | 664,001 | **-81,984 (-11%)** | | **Heap size** | 115 MB | 111 MB | **-4 MB** | | GetterSetter | 34,625 | 13,428 | -21,197 (-61%) | | Function | 221,302 | 197,024 | -24,278 (-11%) | | JSLexicalEnvironment | 70,101 | 44,633 | -25,468 (-36%) | | Structure | 40,254 | 39,762 | -492 |
### Problem The bundler's `__toESM` helper creates a new getter-wrapped proxy object every time a CJS module is imported. In a large app, a popular dependency like React can be imported 600+ times — each creating a fresh object with ~44 getter properties. This produces ~27K unnecessary `GetterSetter` objects, ~25K closures, and ~25K `JSLexicalEnvironment` scope objects at startup. Additionally, `__export` and `__exportValue` use `var`-scoped loop variables captured by setter closures, meaning all setters incorrectly reference the last iterated key (a latent bug). ### Changes 1. **`__toESM`: add WeakMap cache** — deduplicate repeated wrappings of the same CJS module. Two caches (one per `isNodeMode` value) to handle both import modes correctly. 2. **Replace closures with `.bind()`** — `() => obj[key]` becomes `__accessProp.bind(obj, key)`. BoundFunction is cheaper than Function + JSLexicalEnvironment, and frees the for-in `JSPropertyNameEnumerator` from the closure scope. 3. **Fix var-scoping bug in `__export`/`__exportValue`** — setter closures captured a shared `var name` and would all modify the last iterated key. `.bind()` eagerly captures the correct key per iteration. 4. **`__toCommonJS`: `.map()` → `for..of`** — eliminates throwaway array allocation. 5. **`__reExport`: single `getOwnPropertyNames` call** — was calling it twice when `secondTarget` was provided. ### Impact (measured on a ~23MB single-bundle app with 600+ React imports) | Metric | Before | After | Delta | |--------|--------|-------|-------| | **Total objects** | 745,985 | 664,001 | **-81,984 (-11%)** | | **Heap size** | 115 MB | 111 MB | **-4 MB** | | GetterSetter | 34,625 | 13,428 | -21,197 (-61%) | | Function | 221,302 | 197,024 | -24,278 (-11%) | | JSLexicalEnvironment | 70,101 | 44,633 | -25,468 (-36%) | | Structure | 40,254 | 39,762 | -492 |
2 participants
Problem
The bundler's
__toESMhelper creates a new getter-wrapped proxy object every time a CJSmodule is imported. In a large app, a popular dependency like React can be imported 600+
times — each creating a fresh object with ~44 getter properties. This produces ~27K
unnecessary
GetterSetterobjects, ~25K closures, and ~25KJSLexicalEnvironmentscopeobjects at startup.
Additionally,
__exportand__exportValueusevar-scoped loop variables captured bysetter closures, meaning all setters incorrectly reference the last iterated key (a latent
bug).
Changes
__toESM: add WeakMap cache — deduplicate repeated wrappings of the same CJSmodule. Two caches (one per
isNodeModevalue) to handle both import modes correctly..bind()—() => obj[key]becomes__accessProp.bind(obj, key). BoundFunction is cheaper than Function + JSLexicalEnvironment, and frees the for-inJSPropertyNameEnumeratorfrom the closure scope.__export/__exportValue— setter closures captured ashared
var nameand would all modify the last iterated key..bind()eagerly capturesthe correct key per iteration.
__toCommonJS:.map()→for..of— eliminates throwaway array allocation.__reExport: singlegetOwnPropertyNamescall — was calling it twice whensecondTargetwas provided.Impact (measured on a ~23MB single-bundle app with 600+ React imports)