runtime: share one parsed value between import and require() of a JSON/TOML file - #35973
runtime: share one parsed value between import and require() of a JSON/TOML file#35973robobun wants to merge 9 commits into
Conversation
…N/TOML file A .json file that was first imported via ESM and then require()d returned the module namespace (with a self-referencing `default` key and, for arrays, `Array.isArray(req) === false`) instead of the plain parsed data. The ESM default and the require() result were also two distinct objects in both load orders, where Node.js returns one. The ESM data-module path (JSONForObjectLoader / ExportsObject / ExportDefaultObject) built a synthetic module record but never touched require.cache, so a later require() hit the "already in the ESM registry" short-circuit in fetchCommonJSModule and the JS side handed back the namespace object. reconcileDataModuleWithRequireCache now seeds require.cache with the parsed value (or reuses the value an earlier require() already put there), so both loaders expose the same object.
|
Warning Review limit reached
Next review available in: 11 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 (2)
Comment |
|
Updated 3:06 PM PT - Jul 26th, 2026
❌ @robobun, your commit 081bbdd has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35973That installs a local version of the PR into your bun-35973 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
ExportDefaultObject covers HTML bundles, CSS stubs and the file loader; seeding require.cache for those made the inspector report .html routes as CJS modules (BunFrontendDevServer.test.ts). JSONForObjectLoader and ExportsObject cover every data format the reported bug applies to.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
JSMap::get() returns jsUndefined() for a missing key, so the previous
ternary never distinguished "no entry" from "entry is a plain object
the user installed". Leave user-installed entries alone so a mock like
`require.cache[key] = { exports: x }` survives a later ESM import.
… output A custom require.extensions['.json'] handler leaves the cached module with hasEvaluated=false, so the previous gate fell through and replaced the handler's exports with the on-disk parse. The only ways a Module for a data file reaches requireMap before this helper runs are a prior require() or a user-installed entry; in both cases its exports are the value require() would return, so reuse them unconditionally.
|
CI: the diff is green. Remaining red is unrelated:
Ready for review. |
There was a problem hiding this comment.
All four earlier findings are addressed in 081bbdd and I found nothing new on this revision. Deferring to a human for final sign-off since this changes ESM↔CJS cache interop in the core module loader.
What was reviewed:
reconcileDataModuleWithRequireCache: exception-scope discipline matches neighboringfetchESMSourceCodebranches;JSCommonJSModule::create(global, JSString*, exports, hasEvaluated=true, parent=undefined)matches the header overload.- Confirmed the plain-object
require.cacheguard, the droppedhasEvaluatedgate, and the removed dead ternary/unusedfsrequire all landed as described. ExportDefaultObjectbranch is untouched (per the dev-server regression noted in the description); the two seeded call sites coverJSONForObjectLoaderandExportsObjectonly.
Extended reasoning...
Overview
The PR adds a ~25-line helper reconcileDataModuleWithRequireCache in src/jsc/bindings/ModuleLoader.cpp and calls it from the two data-module branches of fetchESMSourceCode (JSONForObjectLoader for .json, ExportsObject for TOML/JSONC/YAML). The helper either reuses an existing require.cache entry's exports or seeds a fresh JSCommonJSModule so that import default and require() of the same data file share one object, matching Node.js. A new 256-line test file with 10 subprocess tests covers both orders, arrays, TOML, require.cache mocking, require.extensions overrides, cache deletion, and namespace shape.
Security risks
None identified. The change reads/writes requireMap (an internal JSMap) keyed by resolved specifier strings and creates JSCommonJSModule instances via the existing overload. No untrusted-size arithmetic, no new parsing, no file I/O; the parsed value already existed and is now shared rather than duplicated.
Level of scrutiny
High. ModuleLoader.cpp is the ESM/CJS interop boundary — changes here affect every data-file load in the runtime. Over three prior review rounds this PR needed two behavior fixes (plain-object require.cache clobber, require.extensions clobber via the hasEvaluated gate) and two dead-code removals, and an earlier iteration broke BunFrontendDevServer.test.ts before ExportDefaultObject was carved out. That iteration history is exactly why a human should give the final look, even though the current revision reads correctly.
Other factors
Exception handling follows local convention (same scope.exception() → tryClearException() → reject(exception) shape as the JSON-parse block immediately above each call site). dynamicDowncast<JSCommonJSModule>(JSValue) matches existing usage in this file (e.g. dynamicDowncast<JSC::Exception>(objectValue)). The PR description reports the new test file passes under BUN_JSC_validateExceptionChecks=1 and lists a dozen adjacent suites re-run green on the debug build; CI on the predecessor commit was green modulo unrelated flakes, and 081bbdd only removed dead code relative to it.
2 participants
A
.jsonfile that is bothimported andrequire()d in one process returned two different objects in Bun, and when the ESM import happened first therequire()result was the module namespace rather than the plain data.Reproduction
For a JSON array it was worse:
Array.isArray(require('./arr.json'))wasfalseafter an ESM import of the same file, because the namespace object (not the array) came back. The reverse order (requirefirst, thenimport) was clean on shape but still produced two distinct objects. Node.js guarantees identity for JSON in both orders.Cause
fetchESMSourceCodehandles JSON/TOML/JSONC/YAML by parsing the value and wrapping it in a synthetic module record (generateJSValueModuleSourceCodeinObjectModule.cpp). It never touchesrequire.cache. A laterrequire()reachesfetchCommonJSModule, sees the specifier already in the ESM registry, returns-1, andoverridableRequirefalls back tonamespace["module.exports"] ?? namespace, which for these records is the namespace itself.A
require()without a prior import takes a different branch (fetchCommonJSModuleNonBuiltin→target.exports = JSON.parse(src)) and is unaffected, but that parse result is private torequire.cacheand never reused by a laterimport.Fix
reconcileDataModuleWithRequireCacheis called from the two data-module branches offetchESMSourceCode(JSONForObjectLoaderfor.json,ExportsObjectfor TOML/JSONC/YAML). It seedsrequire.cache[specifier]with aJSCommonJSModulewhoseexportsis the parsed value, or reuses theexportsan earlierrequire()already put there. Both loaders now hand back the same object, andrequire()returns the plain data because it finds the entry in$requireMapbefore the ESM-registry short-circuit.The
ExportDefaultObjectbranch (HTML bundles, CSS stubs, the file loader) is intentionally left alone: seedingrequire.cachethere made the inspector report.htmldev-server routes as CJS modules.The ESM namespace shape is unchanged (no extra export names), and
delete require.cache[key]still forces a re-read.Verification
USE_SYSTEM_BUN=1 bun test test/js/bun/resolve/json-require-import-identity.test.ts: 2 pass (regression guards) / 6 fail.bun bd test test/js/bun/resolve/json-require-import-identity.test.ts: 8 pass / 0 fail (also underBUN_JSC_validateExceptionChecks=1).Also green on the debug build:
jsonc.test.ts,import-meta.test.js,esModule.test.ts,esModule-annotation.test.js,resolve.test.ts,import-query.test.ts,import-attributes.test.ts,toml/toml.test.js,require-and-import-trailing.test.ts,node-module-module.test.js,require-extensions.test.ts,BunFrontendDevServer.test.ts.Related
#35914 is about a
.jsonimported with and withoutwith { type: 'json' }producing two ESM records; this PR is about the ESM record and the CJS cache not sharing a value. The two are independent.[review] gate passed · iteration 3 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 3
evidence per changed file