Repository navigation
Add Bun.unsafe.ModuleGraph: further instances of the ES module graph in one global object #42754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,11 @@ | |
| return this.$requireNativeModule(id); | ||
| } else { | ||
| const existing = $requireMap.$get(id); | ||
| if (existing && existing.$esModule && this.$moduleGraph !== undefined) { | ||
|
Check failure on line 37 in src/js/builtins/CommonJS.ts
|
||
| // The entry is the host's instance of an ES module. A Bun.unsafe.ModuleGraph's | ||
| // modules get their graph's own, which never enters the require cache. | ||
| return $requireESMExports($requireESM(id, this.$moduleGraph)); | ||
| } | ||
| if (existing) { | ||
| // Scenario where this is necessary: | ||
| // | ||
|
|
@@ -71,10 +76,13 @@ | |
| return Bun.jest(this.filename); | ||
| } | ||
|
|
||
| // To handle import/export cycles, we need to create a module object and put | ||
| // it into the map before we import it. | ||
| // To handle import/export cycles, the module object has to be in the map before | ||
| // the module evaluates. Whether `id` is an ES module is not known yet, and an ES | ||
| // module is not found in the map while it evaluates (a require cycle gets its live | ||
| // namespace from the module loader instead), so $require puts `mod` into the map | ||
| // itself, once `id` turns out to be anything else. | ||
| const mod = $createCommonJSModule(id, {}, false, this); | ||
| $requireMap.$set(id, mod); | ||
| const graph = this.$moduleGraph; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Deferring the Extended reasoning...…Fix: keep re-entrant Base: After this PR: the Verification: normal — narrow edge case, but a real regression the deferral introduces. Base |
||
|
|
||
| var out: LoaderModule | -1; | ||
|
|
||
|
|
@@ -97,7 +105,7 @@ | |
| $argument(1), | ||
| ); | ||
| } catch (E) { | ||
| $assert($requireMap.$get(id) === undefined, "Module " + JSON.stringify(id) + " should no longer be in the map"); | ||
| $assert($requireMap.$get(id) !== mod, "Module " + JSON.stringify(id) + " should no longer be in the map"); | ||
| throw E; | ||
| } | ||
| } else { | ||
|
|
@@ -106,40 +114,17 @@ | |
|
|
||
| // -1 means we need to lookup the module from the ESM registry. | ||
| if (out === -1) { | ||
| try { | ||
| out = $requireESM(id); | ||
| } catch (exception) { | ||
| // Since the ESM code is mostly JS, we need to handle exceptions here. | ||
| $requireMap.$delete(id); | ||
| throw exception; | ||
| } | ||
|
|
||
| const namespace = out; | ||
| // In a require cycle the namespace is live while the module body is still | ||
| // running, so an export named `__esModule` / `module.exports` may be in TDZ. | ||
| let esModule, moduleExports; | ||
| try { | ||
| esModule = namespace.__esModule; | ||
| moduleExports = namespace["module.exports"]; | ||
| } catch {} | ||
| // In Bun, when __esModule is not defined, it's a CustomAccessor on the prototype. | ||
| // Various libraries expect __esModule to be set when using ESM from require(). | ||
| // We don't want to always inject the __esModule export into every module, | ||
| // And creating an Object wrapper causes the actual exports to not be own properties. | ||
| // So instead of either of those, we make it so that the __esModule property can be set at runtime. | ||
| // It only supports "true" and undefined. Anything non-truthy is treated as undefined. | ||
| // https://github.com/oven-sh/bun/issues/14411 | ||
| if (esModule === undefined) { | ||
| try { | ||
| namespace.__esModule = true; | ||
| } catch { | ||
| // https://github.com/oven-sh/bun/issues/17816 | ||
| } | ||
| } | ||
|
|
||
| return (mod.exports = moduleExports ?? namespace); | ||
| const exports = $requireESMExports($requireESM(id, graph)); | ||
| // A Bun.unsafe.ModuleGraph's ES module instances never enter the (shared) require cache. | ||
| if (graph !== undefined) return exports; | ||
| mod.$esModule = true; | ||
| $requireMap.$set(id, mod); | ||
| return (mod.exports = exports); | ||
| } | ||
|
|
||
| // A wrapped Module._extensions handler loaded the graph's instance of an ES module into `mod`. | ||
| if (graph !== undefined && mod.$esModule) return mod.exports; | ||
|
|
||
| const c = $evaluateCommonJSModule(mod, this); | ||
| if (c && c.indexOf(mod) === -1) { | ||
| c.push(mod); | ||
|
|
@@ -171,40 +156,35 @@ | |
| } | ||
|
|
||
| $visibility = "Private"; | ||
| export function loadEsmIntoCjs(resolvedSpecifier: string) { | ||
| export function loadEsmIntoCjs(resolvedSpecifier: string, graph?: object) { | ||
| // The JSC module loader pipeline is now pure C++. $esmLoadSync sets a VM | ||
| // flag that makes the loader's internal promise reactions run immediately | ||
| // (instead of queueing microtasks) whenever the upstream promise is already | ||
| // settled. Because Bun resolves and reads source code synchronously, the | ||
| // entire fetch → parse → link → evaluate chain completes within this call | ||
| // for any module graph that does not use top-level await. | ||
| return $esmLoadSync(resolvedSpecifier); | ||
| return $esmLoadSync(resolvedSpecifier, graph); | ||
| } | ||
|
|
||
| // `graph` is the Bun.unsafe.ModuleGraph whose instance of the module to load, or | ||
| // undefined for the global object's own. | ||
| $visibility = "Private"; | ||
| export function requireESM(this, resolved: string) { | ||
| export function requireESM(this, resolved: string, graph?: object) { | ||
| // `$esmLoadSync` answers from the registry for a record that is already | ||
| // Evaluated, or still Evaluating because this require() sits inside its own | ||
| // evaluation (a require cycle), before it loads anything. | ||
| const exports = $loadEsmIntoCjs(resolved); | ||
| const exports = $loadEsmIntoCjs(resolved, graph); | ||
| if (exports === undefined) { | ||
| throw new TypeError(`require() failed to evaluate module "${resolved}". This is an internal consistentency error.`); | ||
| } | ||
| return exports; | ||
| } | ||
|
|
||
| export function requireESMFromHijackedExtension(this: JSCommonJSModule, id: string) { | ||
| $assert(this); | ||
| let namespace; | ||
| try { | ||
| namespace = $requireESM(id); | ||
| } catch (exception) { | ||
| // Since the ESM code is mostly JS, we need to handle exceptions here. | ||
| $requireMap.$delete(id); | ||
| throw exception; | ||
| } | ||
|
|
||
| // See `overridableRequire`: TDZ-safe reads for the require-cycle case. | ||
| // What require() returns for an ES module's namespace. | ||
| $visibility = "Private"; | ||
| export function requireESMExports(namespace) { | ||
| // In a require cycle the namespace is live while the module body is still | ||
| // running, so an export named `__esModule` / `module.exports` may be in TDZ. | ||
| let esModule, moduleExports; | ||
| try { | ||
| esModule = namespace.__esModule; | ||
|
|
@@ -225,7 +205,21 @@ | |
| } | ||
| } | ||
|
|
||
| this.exports = moduleExports ?? namespace; | ||
| return moduleExports ?? namespace; | ||
| } | ||
|
|
||
| export function requireESMFromHijackedExtension(this: JSCommonJSModule, id: string) { | ||
| $assert(this); | ||
| // The handler ran with `this` in the require cache, as handlers expect. See | ||
| // `overridableRequire`: an ES module is not found there while it evaluates. | ||
| if ($requireMap.$get(id) === this) $requireMap.$delete(id); | ||
| const graph = $requiringModuleGraph(); | ||
| const namespace = $requireESM(id, graph); | ||
|
|
||
| this.$esModule = true; | ||
| this.exports = $requireESMExports(namespace); | ||
| // A Bun.unsafe.ModuleGraph's ES module instances never enter the (shared) require cache. | ||
| if (graph === undefined) $requireMap.$set(id, this); | ||
| } | ||
|
|
||
| $visibility = "Private"; | ||
|
|
@@ -254,6 +248,7 @@ | |
| const namespace = $esmNamespaceForCjs(key); | ||
| if (namespace !== undefined) { | ||
| const mod = $createCommonJSModule(key, namespace, true, undefined); | ||
| mod.$esModule = true; | ||
| $requireMap.$set(key, mod); | ||
| return mod; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix the lint error by reading
this.$moduleGraphonce.oxlint.jsonenablesbun/no-duplicate-conditional-property-accessas an error forsrc/js/**.bun lintscans that tree, and CI runs it in theLint JavaScriptjob. InoverridableRequire, the condition readsthis.$moduleGraphand its body reads it again. The lint job can fail.Hoist the read and reuse
graphin both branches.🛠️ Proposed fix
export function overridableRequire(this: JSCommonJSModule, originalId: string, options?: { paths?: string[] }) { const id = $resolveSync(originalId, this.filename, false, false, options ? options.paths : undefined, this, options); + const graph = this.$moduleGraph; if (id.startsWith("node:")) {Then remove the later duplicate declaration:
const mod = $createCommonJSModule(id, {}, false, this); - const graph = this.$moduleGraph;🧰 Tools
🪛 GitHub Check: Lint JavaScript
[failure] 37-37: bun(no-duplicate-conditional-property-access)
this.$moduleGraphis read in theifcondition and again in the body. Read it into a local first (e.g.const { $moduleGraph } = this) so the property is only accessed once.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed across
b64edbeandc911d77.You were right that the predicate ignored the observed
directoryNameconstraint, and that a retained permit CA can carry a foreign directory-name base while its DNS list still overlaps. The gate now withdraws onstats.caCannotMintUsableLeafbefore any dNSName reasoning, because no amount of DNS overlap changes an unchainable leaf.Two corrections to my first pass at it, both from later review:
directoryNameconstraints use RDN-sequence prefix matching (RFC 5280 4.2.1.10), so a base ofO=VibeTray Capturelegitimately admitsO=VibeTray Capture, CN=VibeTray Capture Leaf. That version withdrew the writer for CAs that work.excludedSubtreesdirectoryNamewas never parsed, so a CA explicitly forbidding the pinned subject read as usable — the direction that actually matters, since we would mint leaves the CA forbids and every handshake would fail at the client with nothing saying why.Both now route through one containment helper and one decision, alongside the certificate-level causes (
cA,keyCertSign, validity, unparseable constraints).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tjmarlin69, the described certificate-constraint changes do not address thisCommonJS.tsfinding.overridableRequirestill readsthis.$moduleGraphin the condition and again in$requireESM(...). The laterconst graph = this.$moduleGraphalso remains below the conditional. Please hoist that declaration before the branch and reusegraph.You are interacting with an AI system.