Stop running builtin modules' lazy accessors when they are imported as ESM - #37525
Conversation
|
Warning Review limit reached
Next review available in: 14 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 (3)
Comment |
|
Status
|
![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. Because it depends on an unmerged companion change in oven-sh/WebKit#408 (currently pinned as a preview build that the description says must be repointed before landing) and changes the module-loader path every builtin ESM import goes through, a human look is still warranted.
Checked: exception-scope handling around the new getOwnPropertySlot / slot.getValue calls; that data-property exports still snapshot exactly as before and default is still appended; that the hasOwn == false fallback preserves the old object->get behaviour; that require() of builtins doesn't route through this generator; and that the test file's per-process isolation, test.concurrent, and pipe-draining match harness conventions.
Extended reasoning...
Overview
This PR stops generateInternalModuleSourceCode in src/jsc/bindings/ModuleLoader.cpp from eagerly invoking accessor properties on builtin modules' exports objects when they are imported as ESM. Accessors are instead declared as lazy exports (empty JSValue in the export values buffer) and the exports object is returned as the "source" for later materialization. The engine machinery that actually materializes those slots on first bind/read lives in oven-sh/WebKit#408, which this PR pulls in by pinning WEBKIT_VERSION to a preview autobuild tag. A new 8-case test file exercises named imports, import *, import defer *, dynamic import, re-exports, function-typed exports objects, and spyOn/mock.module interaction.
Security risks
None identified. The change is scoped to the internal-module-registry ESM path (Bun's own src/js builtins); user loader: "object" plugins and user CJS-from-ESM are explicitly left on the eager path. No untrusted input reaches the new code.
Level of scrutiny
High. This is the hot path for every import of a node:* / bun:* builtin, and correctness depends on engine changes that are not in this diff — GC rooting of the returned source object across the module record's lifetime, TDZ-slot handling in JSModuleNamespaceObject, and the IC/DFG claims in the description all live in the WebKit PR. The bun-side diff itself is small and looks correct (exception checks after each fallible call, data properties unchanged, hasLazyExports gates returning the object so accessor-free builtins produce byte-identical records), but I can't verify the WebKit half from here.
Other factors
- The WebKit pin is explicitly a preview build with a comment saying to repoint it at a main sha once oven-sh/WebKit#408 merges. That's a landing-order decision a maintainer should sign off on.
- The observable-timing change (accessor sampled at first bind rather than at import) is argued in the description as strictly-no-worse than Node's behaviour, but it is a user-visible semantic change to when builtin getters run.
- Test coverage is thorough for the binding paths and follows harness conventions (per-process isolation,
test.concurrent, concurrent pipe drains,tempDir). The tests can't run without the preview WebKit, so CI is the real verification here.
|
On the two points the review flags:
|
|
@robobun rebase once WebKit PR finishes building |
|
oven-sh/WebKit#408 merged as 7b763944f0ec; its release build on WebKit main is still running. Once the autobuild-7b763944f0ec... release is up I will rebase onto main and point WEBKIT_VERSION at that sha. |
45b4247 to
1b92c69
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 5:16 PM PT - Aug 11th, 2026
✅ @robobun, your commit 435641a6bd5250bad6ac6b263b50dec73cc2a9d3 passed in 🧪 To try this PR locally: bunx bun-pr 37525That installs a local version of the PR into your bun-37525 --bun |
1b92c69 to
1f03b20
Compare
…s ESM generateInternalModuleSourceCode turns a builtin's CommonJS exports object into the synthetic module ESM importers see. It read every own property with get(), so the accessors builtins use to defer loading parts of themselves (node:fs's stream classes, which pull in the whole node:stream stack; tls.rootCertificates; http.globalAgent; timers.promises; ...) all ran on every import of the module, and only require() callers got the laziness. Data properties are still snapshotted at import. Accessors are now declared as lazy exports of the synthetic record and the exports object is handed over as their source; JSC reads the property the first time an importer links the binding or reads it off the namespace object. A builtin whose exports object has no accessors produces the same record as before. The engine half is oven-sh/WebKit#408; WEBKIT_VERSION moves from 09e477744721 to its merge commit 7b763944f0ec, which is the only change between the two.
1f03b20 to
435641a
Compare
|
Rebased onto main (which now pins 09e477744721 via #37352) and pointed Checked locally against the new tarball: the test file passes with the diff and 7 of its 8 cases fail with main's |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a coordinated engine + runtime change to how every builtin's ESM record is built — with a WebKit bump whose GC-rooting and IC/TDZ semantics live in oven-sh/WebKit#408 — a human look is still warranted before landing.
Checked on the bun side: exception handling in the rewritten generateInternalModuleSourceCode loop (getOwnPropertySlot / slot.getValue / fallback get each have RETURN_IF_EXCEPTION); !slot.isValue() correctly catches accessor and custom slots while data properties still snapshot eagerly; returning nullptr when no accessors were seen keeps the record identical to before. The test file spawns each case in its own process, drains stdout/stderr/exited concurrently, and covers named/namespace/defer/re-export/mock.module paths. Also confirmed generateObjectModuleSourceCode (plugin loader: "object") and JSON/TOML paths still use plain SyntheticSourceProvider::create, so user-object getter timing is unchanged.
Extended reasoning...
Overview
The PR changes generateInternalModuleSourceCode in src/jsc/bindings/ModuleLoader.cpp so that when a src/js builtin is imported as ESM, own accessor properties on its exports object are declared as lazy exports (empty JSValue slots) instead of being read eagerly via object->get(). It switches the return type to LazySyntheticSourceGenerator (returning the exports object as the lazy source, or nullptr if no accessors were seen) and the call site to SyntheticSourceProvider::createWithLazyExports. WEBKIT_VERSION moves one commit forward to pick up the engine half (oven-sh/WebKit#408). A new 232-line test file exercises named imports, import *, import defer *, dynamic import(), export * / renamed re-exports, function-typed exports objects (node:assert), and spyOn/mock.module interaction.
Security risks
None identified. The change is internal to how Bun's own builtin modules are exposed to ESM; user-supplied objects (plugin loader: "object", user CJS) continue through the unchanged generateObjectModuleSourceCode / SyntheticSourceProvider::create path. No new untrusted-input parsing.
Level of scrutiny
High. fetchESMSourceCode's InternalModuleRegistryFlag branch is on the path of every import 'node:*', and the correctness of leaving slots in TDZ until first binding depends entirely on engine-side invariants (source-object GC visiting in SyntheticModuleRecord, materialization in initializeEnvironment / JSModuleNamespaceObject::getOwnPropertySlotCommon / WebAssemblyModuleRecord, namespace IC bailing on empty slots, DFG folding only through installed ICs) that live in a separate repository's diff. The bun-side C++ is small and looks correct in isolation — each fallible call has RETURN_IF_EXCEPTION, PropertySlot is used with InternalMethodType::GetOwnProperty, and the !slot.isValue() predicate matches JSC's accessor/custom-getter classification — but the overall behaviour cannot be signed off without also reviewing the WebKit half.
Other factors
- The change intentionally shifts when an accessor export is sampled (import time → first binding). The description argues this is never observable as staler than before, and the tests pin identity to
require()'s value on every path, but it is a semantic change to module-loading behaviour that a maintainer should acknowledge. - Jarred-Sumner has engaged (asked for the rebase after the WebKit build) but has not yet approved.
- The WebKit pin move, while a single commit, means every platform's prebuilt tarball changes; CI on the pinned build (#92626) should be green before merge.
- Test quality is good: per-process isolation (a binding materializes once), concurrent pipe draining,
test.concurrent, and 7/8 cases verified to fail on the old path.
|
Nothing to change from that review. The remaining item it lists, CI on the final pin (7b763944f0ec), is running as build 92642 for 435641a; the comment-cop threads from the earlier pushes are resolved. |
… whole Bun object (#37714) Follow-up to #37525, which made the ES module view of the `src/js` builtins declare their accessors lazily. The `"bun"` module is not one of those: it is a native module (`generateNativeModule_BunObject` in `src/jsc/bindings/BunObject.cpp`), and its generator still built the record eagerly: - `object->reifyAllStaticProperties()` runs every `PropertyCallback` in `bunObjectTable` (about 60 of them: `$`, `SQL`/`sql`/`postgres`, `S3Client`/`s3`, `RedisClient`/`redis`, `Glob`, `TOML`, `WebView`, `secrets`, `stdin`/`stdout`/`stderr`, ...) and stores each result on the object, so that line alone constructs every lazy `Bun.*` property. - `exportBunObject` then called `get()` on each of them to fill the export values. Note that the most common forms never reach this code: the transpiler rewrites a literal `import { write } from "bun"`, `import * as b from "bun"`, `import("bun")` and `require("bun")` into reads of `globalThis.Bun` (`src/js_printer/lib.rs`, `ImportRecordTag::Bun`), which is why `import { write } from "bun"` only ever constructed `write`. The generator runs for `export * from "bun"` / `export { x } from "bun"` and for `import()` with a non-literal specifier, and every one of those constructed all 115 static properties. ## Repro ```js // reexport.mjs export * from "bun"; // entry.mjs import { describe } from "bun:jsc"; import { write } from "./reexport.mjs"; // describe() dumps the Structure, i.e. the static-table entries that have been constructed so far. console.log(describe(Bun).match(/\{[^}]*\}/)[0].split(",").length); ``` Before: 116 entries (`Symbol.toStringTag` plus all 115). After: 2 (`Symbol.toStringTag` and `write`). A non-literal `import(specifier)` of `"bun"` behaves the same way. A second consequence of the eager path: a property callback that throws fails the whole module. `Bun.redis` builds the default client from `REDIS_URL` and throws on an invalid URL, so with `REDIS_URL='http://[::1'` the entry above died at load with `TypeError: Invalid URL format` even though it only wanted `write`. (The `DECLARE_TOP_EXCEPTION_SCOPE` in `exportBunObject` that turned getter errors into `undefined` never got to run for these, because `reifyAllStaticProperties()` propagated first.) ## Fix `generateNativeModule_BunObject` now has the `LazySyntheticSourceGenerator` signature from #37525. It lists the object's own property names (`getOwnNonIndexPropertyNames` includes the static table entries that have not been reified, so dropping `reifyAllStaticProperties()` leaves the export list unchanged, `DontEnum` entries excluded as before), provides `default` = the Bun object, appends an empty `JSValue` for every other export, and returns the Bun object as the source. JSC's `materializeLazyExport` then reads `Bun[name]` the first time something binds to that export: when an importer links a named import of it (directly or through `export *` / `export { x as y } from`), or when it is read off a namespace object. Reading it reifies exactly that one property on the Bun object, the same thing a direct `Bun[name]` access does, so the binding is identical to the property (`main`, the one `CustomAccessor`, is read through its getter like before). Plumbing: the native module list in `NativeModuleList.h` gets a separate `BUN_FOREACH_LAZY_ESM_NATIVE_MODULE` group for generators with this signature (the codegen scanner numbers modules by their order in the file, and `bun` was already last, so the ids do not move); `_NativeModule.h` forward-declares that group with the new return type; `ModuleLoader.cpp` dispatches it through `SyntheticSourceProvider::createWithLazyExports`; and `generateNativeModule` in `InternalModuleRegistry.cpp` (the generated `createInternalModuleById` case, which only needs the default export) becomes a template so it accepts both signatures. ### Behaviour differences - When a `Bun.*` value is sampled: at first binding rather than at module load. The affected values are constructed on first read and then fixed, so the same object is handed out either way; for `main` (a live getter) a re-export now samples the value when it is first bound rather than when the module loads, which is the same behaviour #37525 gave the builtins' accessors. - A throwing callback now throws from whatever binds to that export (the import that links it, or the namespace read, including `Object.keys(ns)` / `console.log(ns)` since those read every export), and the other exports keep working. Previously it failed the module. Of the current callbacks, `Bun.redis` throws on an invalid `REDIS_URL` / `VALKEY_URL`, `Bun.embeddedFiles` can throw on OOM, and `Bun.$`, `Bun.sql`, `Bun.postgres`, `Bun.SQL` would throw if their builtin failed to load; `Bun.s3` reports its error as unhandled and yields `undefined` instead. The error surfacing on the `redis` binding is the same error a direct `Bun.redis` read produces, which the new test pins. - `mock.module("bun", ...)` on an already loaded record writes into the binding with `overrideExportValue`; `materializeLazyExport` is a no-op for a slot that has a value, so the mock wins and the real property is never constructed. - No bulk reification means nothing here runs callbacks back to back without exception checks, which is what #33150 was fixing in the eager path; this supersedes it. Checked with `BUN_JSC_validateExceptionChecks=1` on the re-export, the non-literal `import()`, the throwing `redis` case, and a full `Object.keys(ns)` materialization. ## Tests Added to `test/js/bun/resolve/builtin-esm-lazy-exports.test.ts` (the file from #37525), each in its own process. The readout is `bun:jsc`'s `describe(Bun)`, filtered to a sample of the lazy properties plus `write`; `Object.getOwnPropertyDescriptor` cannot be used because it reifies the property it is asked about. - `export *` / `export { x as y }` / `export { default as ... }` re-exports: linking `import { write }` constructs only `write`; `in` constructs nothing; reading the renamed export constructs `Glob`; reading through the star constructs `SQL`; each is identical to the `Bun.*` value and stable across reads; `default` is the Bun object. - Non-literal `import()`: nothing constructed on import; `Reflect.ownKeys` of the namespace equals `Object.keys(Bun)` plus `default` and constructs nothing; reading `TOML` constructs only `TOML`; every export is `===` the corresponding `Bun` property (which constructs everything). The specifier is imported from the helper module because a `const` holding it can get inlined into a literal `import("bun")` in some files. - `REDIS_URL` set to an invalid URL: the module still loads, `reexported.redis` throws the same error as `Bun.redis`, other exports work. - `mock.module("bun", () => ({ SQL: "mocked" }))` in a `bun test` child after the re-export was imported: the namespace sees the mock, `SQL` is never constructed, `Bun.SQL` is intact. All four fail on the merge base (everything shows up as constructed after import; the `REDIS_URL` case fails at load), and the eight existing cases in the file pass on both. Also run on this build: `test/js/bun/util/BunObject.test.ts` (which `console.log`s a non-literal `import()` namespace of `"bun"` and compares every property), `test/js/bun/resolve/`, `test/js/bun/test/mock/`, `test/js/node/stubs.test.js`, `node-module-module.test.js`, and `test-process-get-builtin.mjs`; all green apart from `load-same-js-file-a-lot.test.ts`, which times out identically on the unmodified build in this environment. ## Startup Wall time of `bun file.mjs`, min of 10 runs after a warm-up, debug (ASAN) builds of the merge base and of this branch on the same machine: | file | before | after | | --- | ---: | ---: | | empty module | 273 ms | 264 ms | | `import { write } from "bun"; write.length` | 277 ms | 251 ms | | same, but `write` comes from a module doing `export * from "bun"` | 555 ms | 279 ms | The literal form is rewritten by the transpiler and was already at the floor; the re-export form loses the roughly 280 ms (debug build) it spent constructing the object. Release numbers to follow in a comment.
Follow-up to #37714 (the `"bun"` module) and #37525 (the `src/js` builtins): the two remaining native ES modules that mirror an existing object, `node:process` and `node:module`, still built their records eagerly. `generateNativeModule_NodeProcess` called `get()` on every enumerable property of `process` and its prototype chain, and `generateNativeModule_NodeModule` on every entry of the Module constructor's static table. Unlike `"bun"`, these imports are not rewritten by the transpiler, so every `import process from "node:process"` (a very common line in published ESM packages) and every `import { createRequire } from "node:module"` paid for it. ## Repro ```js // entry.mjs import { describe } from "bun:jsc"; import process from "node:process"; import { createRequire } from "node:module"; // describe() dumps an object's Structure, i.e. which of its static table entries have been constructed so far. const entries = object => describe(object).match(/\{[^}]*\}/)[0].split(",").length; console.log(entries(process), entries(globalThis.process.getBuiltinModule("node:module"))); ``` Before: `86 27`. For `process` that is everything in the table that can be reified, including `stdout`, `stderr` and `stdin`, which construct the stdio streams and load `node:tty` / `node:stream` to do it, plus `config`, `release`, `allowedNodeEnvironmentFlags`, `versions`, ...; for `Module` it is `_cache`, `builtinModules`, `globalPaths`, `SourceMap`, the `wrapper` proxy and the rest of the table. After: `2 3`, i.e. only what is there before user code runs (`Symbol.toStringTag` and `_exiting` on `process`, `length` and `name` on `Module`) plus `createRequire`, the one thing the file imported. On this machine (release build of 1.4.0, min of 20 runs), `import process from "node:process"` added about 18 ms to the startup of an otherwise empty file, about the same as touching `process.stdout` does; with this change the import itself is free and that cost moves to whoever actually reads `stdout`. Measured numbers (release 1.4.0 for the eager cost, a debug build of this branch for the new one) are in a comment below. ## Fix Both generators move to the `BUN_FOREACH_LAZY_ESM_NATIVE_MODULE` group added in #37714 (the entries keep their position in `NativeModuleList.h`, so the generated ids do not change) and share one helper, `exportObjectProperties()` in `_NativeModule.h`, which the `"bun"` generator now uses too. For each name the caller wants exported it does the split #37525 does for the `src/js` builtins: a value that is already stored on the object (`getDirect()` finds a plain value) is exported as is, anything else is declared without a value and JSC's `materializeLazyExport` reads `object[name]` the first time something binds to it. "Anything else" covers a static table entry nobody has read yet (the expensive case), an accessor (`process.argv`, `process.title`, `Module._resolveFilename`, `Module.wrapper`), and a property inherited from the prototype chain (the EventEmitter methods of `process`). The generators themselves only decide the name list, which is what keeps the export lists identical to before: - `node:process`: `getPropertyNames()` on `process`, as before, so the inherited EventEmitter methods (`on`, `emit`, ...) stay exports, and a data property assigned onto `process` before the module is loaded is still exported and still snapshotted at load (`process.test.js` has a test for exactly that, with `default` on top, which keeps being skipped in favour of the object). - `node:module`: the static table, as before, so `length`, `name` and anything user code assigned onto `Module` stay out. - `"bun"`: `getOwnNonIndexPropertyNames()`, as in #37714. The only difference from #37714 is that a `Bun.*` property something already read before the module loads is now snapshotted instead of declared lazily; it is the same object either way. The generators no longer run any getters, so the `TopExceptionScope` handling that turned a throwing getter into an `undefined` export (and the comments about not bulk-reifying because of the exception-check verifier) go away with them. A getter that throws now throws from whatever binds to that export, and a termination arriving while a binding is being materialized propagates from there, the same way #37714 describes for `"bun"`. The sampling-time difference is also the same as there: an accessor export is read when it is first bound rather than when the module loads, which for the first importer is the same moment. ## Tests Added to `test/js/bun/resolve/builtin-esm-lazy-exports.test.ts`, same shape as the `"bun"` cases (each in its own process, readout is `describe()` of the object filtered to a watched sample of names; `Object.keys` / `Reflect.ownKeys` are used for the export-list checks because `for...in` reifies every static property of the object it enumerates): - `node:process`: linking `import proc, { on, release }` constructs exactly `release`; `on` is the inherited method; the export list equals the enumerable names of `process` and its prototype chain plus `default`; listing it constructs nothing; reading `stdout` off the namespace constructs exactly `stdout` and is the real stream; `argv` (an accessor) binds to `process.argv`. - `node:process`: a data property assigned before the load is exported with its value at load time, and one assigned afterwards is not exported (the part of the behaviour this keeps from the eager version). - `node:module`: linking `import Module, { createRequire }` constructs exactly `createRequire` (and it works); the export list equals `Object.keys(Module)` plus `default`; reading `builtinModules` constructs exactly that and is the same array; `_resolveFilename` binds to the accessor's value. The two "linking constructs ..." cases fail on main (everything watched shows up as constructed after import); the snapshot case and the 12 existing cases in the file pass on both. Also run on this build: `test/js/node/process/` (`process.test.js`, `process-stdio`, `process-on`, `call-constructor`, which imports `node:process` as ESM), `test/js/node/module/`, `test/js/node/events/event-emitter.test.ts`, `test/js/bun/util/BunObject.test.ts`, `test/js/bun/test/mock/`, `stubs.test.js`, `require-esm-transitive-tla` and `import-meta-resolve`; green except for two tests that fail identically without this change in this environment (`process.test.js` "process" wants `$USER` set, and `process-args.test.js` spawns 100 debug processes inside a 5 s timeout). `BUN_JSC_validateExceptionChecks=1` is clean for importing both modules and for reading every export of both namespaces.
Builtin ESM records declare accessor-backed exports lazily (#37525); the binding slot stays empty until something binds to it. Snapshotting the original through the namespace object read the slot, which ran the getter and defeated the laziness mock.module() is supposed to preserve. Read the binding's slot directly instead. A materialized value is snapshotted as before. An empty slot is recorded against the record's default export (the builtin's exports object, which is what the engine reads lazy exports from), and mock.restore() reads the real value off it at that point, so the getter runs when the user restores, not when they mock.
2 participants
Builtins implemented in
src/jsare handed to ES module importers bygenerateInternalModuleSourceCode(src/jsc/bindings/ModuleLoader.cpp), which snapshots the builtin's CommonJS exports object into a synthetic module record. It did that withobject->get()on every own enumerable property, so every accessor on the exports object ran at import time. Those accessors are the builtins' lazy-loading mechanism, which means they only ever helpedrequire()callers:node:fs:ReadStream,WriteStream,FileReadStream,FileWriteStream,Utf8Streameachrequire("internal/fs/streams"), which loads the wholenode:streamstack.node:tls:rootCertificatesparses the bundled CA store,DEFAULT_CIPHERSqueries BoringSSL.node:http:globalAgentinstantiates the agent.node:timers/node:stream:promisesloadtimers/promisesandstream/promises.node:assert:AssertionErrorloadsinternal/assert/assertion_error.node:repl,node:events,node:buffer,node:oshave a few more.#35541 is about to turn more of these into accessors (
fs.promisesamong them), which this path would immediately defeat for the most common import style there is.Repro
bun 1.4.0 (release):
esm node:fs 13.3 ms; internal/fs/streams afterwards 0.2 ms(already loaded) vsrequire node:fs 7.8 ms; internal/fs/streams afterwards 5.0 ms. On a debug build the difference is ~370ms per process, andtest/harness.tsdoesimport fs from "node:fs", so every test file paid it.The directly observable form, which the tests use: the
node:fsgetters replace themselves with data properties when they run, so afterimport fs from "node:fs",Object.getOwnPropertyDescriptor(fs, "ReadStream")had avalueinstead of aget.Fix
An ES module binding cannot be made lazy from the embedder side: once the record exists, named imports read the exporting environment's slot directly (
ModuleVar) and namespace reads go throughgetValue()inJSModuleNamespaceObject, so the slot has to hold the value before anything binds to it. The engine half is oven-sh/WebKit#408, merged as 7b763944f0ec.WEBKIT_VERSIONmoves from 09e477744721 (what main pins) to that sha; it is the only commit between the two:SyntheticModuleRecord::tryCreateWithExportNamesAndValues()gets an overload taking a source object; an export whose value is the emptyJSValueis declared but left in TDZ.materializeLazyExport()fills such a slot fromsource[name]the first time something binds to it:CyclicModuleRecord::initializeEnvironmentwhen an importer links a named import that resolves to it (directly or throughexport */export { x } fromchains),JSModuleNamespaceObject::getOwnPropertySlotCommonwhen a namespace read finds the slot empty (it then re-reads and installs the value the same way as before, so the namespace IC still applies), andWebAssemblyModuleRecord::initializeImports, which snapshots imported bindings directly (unreachable in bun today, reachable once runtime: implement WebAssembly/ESM integration for .wasm imports #35587 lands). It is a no-op for a slot that already has a value, sooverrideExportValue(whatmock.moduleandspyOnuse) keeps winning,*namespace*is skipped, a getter that re-enters keeps the first value that landed, and an exception from the getter propagates and leaves the slot for the next attempt. The write uses the samesymbolTablePutTouchWatchpointSetthe eager path used for its one write, so IC / DFG behaviour after materialization is identical to an eagerly built record; the baseline namespace IC already bails to the slow path on an empty slot and the DFG only folds loads that went through an installed IC.SyntheticSourceProvider::createWithLazyExports()takes a generator that returns the source object, andmakeModulepasses it through. Records built any other way (JSON modules,vm.SyntheticModule, every existingcreate()caller) have no source object and take a pointer test on the new paths.The bun half is
generateInternalModuleSourceCode: it now looks each property up withgetOwnPropertySlot, snapshots data properties exactly as before, declares everything else (accessors) as a lazy export, and returns the exports object as the source. A builtin without accessors (most of them) produces a record identical to the old one.require()of a builtin never went through this function (fetchCommonJSModulereturns the registry object), so it is unaffected. The engine change is inert without this opt-in: building the new WebKit with the oldModuleLoader.cppstill fails the new tests the same way the release does.Why this is the right behaviour
Node builds the ESM facade of a builtin by reading the getters too, so the old behaviour was not wrong, just expensive, and there is no compat reason to keep it. The only observable difference is when an accessor export is sampled: at import before, at the point something first binds to it now. Data properties still snapshot at import. A namespace export or named import of an accessor is a snapshot either way (that is what
module.syncBuiltinESMExports()exists for), and the later sample is never staler than the old one. Everything an importer can do with the export resolves to the same objectrequire()would hand out, which is what the tests check for each binding path.Object.keys(ns),hasOwnProperty, spread, andconsole.log(ns)still materialize what they touch, because[[GetOwnProperty]]has to produce the value;in([[HasProperty]]) does not. Pluginloader: "object"modules and user CommonJS imported from ESM are deliberately not touched: those expose user objects whose getter timing is user-visible (#36677 is about that), whereas builtins' accessors are our own lazy-loading idiom.Tests
test/js/bun/resolve/builtin-esm-lazy-exports.test.ts, next to the other module-system behaviour tests in that directory. Each case runs in its own process (a binding can only be materialized once) and checks both that importing left thenode:fsaccessors untouched and that the binding is the real class once used, for: default + named data import, named import of an accessor (exactly that one materializes),import * as,import defer * as, dynamicimport()includingObject.keysand that the export list is unchanged,export *andexport { x as y } fromre-exports (named import through the star and a read off the re-exporter's namespace, which exercises materializing on a record other than the namespace's own), accessors onnode:assert(a function exports object),node:timersandnode:stream, andspyOn/mock.moduleon an already imported builtin. 7 of the 8 cases fail on the current eager code (everything reportsvalue); theassert/timers/streamidentity case is a regression guard.With the fix, the probe above reports
internal/fs/streamsas not loaded afterimport("node:fs")(debug build: 338ms to load it afterwards in both modes, versus 10ms before because the import had already loaded it). Also run on the new build:test/js/node/fs/fs.test.ts(covers the existingexport { ReadStream, WriteStream } from "node:fs"andexport * from "node:fs"fixtures), thetest/js/bun/resolvemodule tests,test/js/bun/test/mock/mock-module*.test.ts,node-module-module.test.js,buffer-inspectmaxbytes.test.ts(which pins that a named import of an accessor is a snapshot), and the events / os / timers / assert / tls-internals / node-http suites, all green apart from two failures that are identical on the released binary in this environment (os.userInfo, the http proxy test).[decide:webkit] gate passed · iteration 0 · 3 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