perf(ext): convert ext/fetch JS sources to lazy-loaded scripts#33784
Conversation
…ernals bridge These are mutable variables set during bootstrap(), after the bridge object is created. A plain property captures the initial value (false/[]), so use getters to always read the current value.
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
Substance follows the established lazy_loaded_js pattern from #33760/#33778/#33779/#33780 — IIFE wrap, globalThis.__bootstrap (or in this case internals for cross-extension symbol bridges), core.loadExtScript() for already-converted extensions, return {...} instead of export {...}. The 8 ext/fetch JS files migrate cleanly to the new shape.
Real regression in OTEL: ESM live-binding semantics lost in destructuring
All 10 test specs debug shards across all 4 platforms fail on specs::cli::otel_basic::basic_grpc with the same diff:
Expected: "kind": 3, (SPAN_KIND_CLIENT — outgoing fetch)
Actual: "kind": 2, (SPAN_KIND_SERVER — wrong)
The root cause is the telemetry bridge in ext/fetch/26_fetch.js:347–360:
const {
builtinTracer,
ContextManager,
enterSpan,
PROPAGATORS,
restoreSnapshot,
TRACING_ENABLED,
} = internals.__telemetry;Destructured from internals.__telemetry, which ext/telemetry/telemetry.ts:1915–1925 sets up as:
internals.__telemetry = {
builtinTracer,
ContextManager,
enterSpan,
get PROPAGATORS() { return PROPAGATORS; }, // live getter
restoreSnapshot,
get TRACING_ENABLED() { return TRACING_ENABLED; }, // live getter
};The get accessors were added correctly because PROPAGATORS and TRACING_ENABLED are mutable module bindings — they're populated later at bootstrap (when OTEL is enabled via flags / env vars). ESM import would have given the consumer a live binding that always reflects the current value. But destructuring const { PROPAGATORS, TRACING_ENABLED } = internals.__telemetry; invokes the getters exactly once at destructuring time, then captures the returned value — which is the pre-bootstrap default (undefined or the initial value).
Downstream effects:
TRACING_ENABLEDevaluates as falsy in the fetch instrumentation path → the wrong code branch fires → the span gets the wrong kind → test fails with2 !== 3.
Fix shape
Two options that preserve the live-binding semantic:
-
Don't destructure, access through the live object on each use:
const telemetry = internals.__telemetry; ... if (!telemetry.TRACING_ENABLED) return; const tracer = telemetry.builtinTracer();
Each property access goes through the getter, so reads always see the current value. The downside is more verbose call sites.
-
Convert
PROPAGATORSandTRACING_ENABLEDfromletbindings to functions (isTracingEnabled(),getPropagators()). The bridge then exposes plain function references that closure over the mutable state. Destructuring is safe again and the call sites stay readable.
Option 2 is the cleaner long-term shape since it makes the live-binding requirement explicit at the API. Option 1 is the smaller diff for unblocking this PR.
The other already-bridged getters (__telemetry.builtinTracer, enterSpan, restoreSnapshot, ContextManager, updateSpanFromClientResponse, etc.) are function references and work fine through destructuring — only the two non-function let bindings have this pitfall.
Other notes
- The body's claim that this also drops the
internals.__fetchRequest/Response/Headersbridges from #33778 is right and verifiable from the diff —ext/cache/01_cache.jsnow usescore.loadExtScript()directly. Net cleanup. - 100/124 success at this head, 13 in flight, 10 failures (all the same OTEL kind mismatch).
Destructuring from the internals bridge object captures the value at load time, losing the live binding that ESM imports provide. Access through the object reference so the getter is called each time.
Follow-up to #33784. Converts `ext/fs/30_fs.js` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`), eliminating ESM module resolution overhead. Updates three consumers to use `core.loadExtScript()`: - `ext/process/40_process.js` (`FsFile` import) - `ext/node/polyfills/_process/process.ts` (`fs` namespace import) - `runtime/js/90_deno_ns.js` (`fs` namespace import) No cross-extension blockers -- `30_fs.js` already uses `core.loadExtScript()` for its `ext:deno_io` and `ext:deno_web` dependencies (converted in #33798).
… scripts (#33817) - Convert `ext/process/40_process.js` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`) - Convert `ext/http/00_serve.ts`, `ext/http/01_http.js`, `ext/http/02_websocket.ts` from ESM to IIFE-wrapped lazy-loaded scripts - Update all consumers (`runtime/js/90_deno_ns.js`, `runtime/js/99_main.js`, `ext/node/polyfills/child_process.ts`, `ext/node/polyfills/internal/child_process.ts`) to use `core.loadExtScript()` instead of ESM imports - Replace `import.meta.log` calls with `internals.log` in `00_serve.ts` Follows the same pattern established in #33778, #33780, #33784, #33800, and #33801.
…ripts (#33818) ## Summary - Convert `ext/kv/01_db.ts` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`) - Convert `ext/webgpu/00_init.js` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`) - Update all consumers (`runtime/js/90_deno_ns.js`, `runtime/js/98_global_scope_shared.js`, `runtime/js/98_global_scope_window.js`, `runtime/js/98_global_scope_worker.js`) to use `core.loadExtScript()` instead of ESM imports - Replace `import.meta.log` call with `internals.log` in `01_db.ts` Follows the same pattern established in #33778, #33780, #33784, #33800, #33801, and #33817.
…and#33784) Follow-up to denoland#33760 and denoland#33779. Converts all 8 ESM JS sources in `ext/fetch` to IIFE-wrapped lazy-loaded scripts (`lazy_loaded_js`). - `20_headers.js`, `21_formdata.js`, `22_body.js`, `22_http_client.js`, `23_request.js`, `23_response.js`, `26_fetch.js`, `27_eventsource.js` - `26_fetch.js` imports from `ext/telemetry` (TypeScript ESM, can't be converted yet) -- bridged via `internals.__telemetry` / `internals.__telemetryUtil` set in the telemetry source files - All other cross-extension deps use `core.loadExtScript()` (ext/web, ext/webidl, ext/net are already `lazy_loaded_js`) This also eliminates the `internals.__fetchRequest/Response/Headers` bridges from denoland#33778 since `ext/cache` can now use `core.loadExtScript()` for ext/fetch directly.
…d#33800) Follow-up to denoland#33784. Converts `ext/fs/30_fs.js` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`), eliminating ESM module resolution overhead. Updates three consumers to use `core.loadExtScript()`: - `ext/process/40_process.js` (`FsFile` import) - `ext/node/polyfills/_process/process.ts` (`fs` namespace import) - `runtime/js/90_deno_ns.js` (`fs` namespace import) No cross-extension blockers -- `30_fs.js` already uses `core.loadExtScript()` for its `ext:deno_io` and `ext:deno_web` dependencies (converted in denoland#33798).
… scripts (denoland#33817) - Convert `ext/process/40_process.js` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`) - Convert `ext/http/00_serve.ts`, `ext/http/01_http.js`, `ext/http/02_websocket.ts` from ESM to IIFE-wrapped lazy-loaded scripts - Update all consumers (`runtime/js/90_deno_ns.js`, `runtime/js/99_main.js`, `ext/node/polyfills/child_process.ts`, `ext/node/polyfills/internal/child_process.ts`) to use `core.loadExtScript()` instead of ESM imports - Replace `import.meta.log` calls with `internals.log` in `00_serve.ts` Follows the same pattern established in denoland#33778, denoland#33780, denoland#33784, denoland#33800, and denoland#33801.
…ripts (denoland#33818) ## Summary - Convert `ext/kv/01_db.ts` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`) - Convert `ext/webgpu/00_init.js` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`) - Update all consumers (`runtime/js/90_deno_ns.js`, `runtime/js/98_global_scope_shared.js`, `runtime/js/98_global_scope_window.js`, `runtime/js/98_global_scope_worker.js`) to use `core.loadExtScript()` instead of ESM imports - Replace `import.meta.log` call with `internals.log` in `01_db.ts` Follows the same pattern established in denoland#33778, denoland#33780, denoland#33784, denoland#33800, denoland#33801, and denoland#33817.
2 participants
Summary
Follow-up to #33760 and #33779. Converts all 8 ESM JS sources in
ext/fetchto IIFE-wrapped lazy-loaded scripts (lazy_loaded_js).20_headers.js,21_formdata.js,22_body.js,22_http_client.js,23_request.js,23_response.js,26_fetch.js,27_eventsource.js26_fetch.jsimports fromext/telemetry(TypeScript ESM, can't be converted yet) -- bridged viainternals.__telemetry/internals.__telemetryUtilset in the telemetry source filescore.loadExtScript()(ext/web, ext/webidl, ext/net are alreadylazy_loaded_js)This also eliminates the
internals.__fetchRequest/Response/Headersbridges from #33778 sinceext/cachecan now usecore.loadExtScript()for ext/fetch directly.Test plan
cargo build --bin denosucceedsfetch,Request,Response,Headers,EventSourceall work