Skip to content

chore: remove EnvironmentOptions::experimental_fetch dependency#51923

Merged
jkleinsc merged 4 commits into
electron:mainfrom
felixrieseberg:felixr/remove-experimental-fetch-option
Jun 16, 2026
Merged

chore: remove EnvironmentOptions::experimental_fetch dependency#51923
jkleinsc merged 4 commits into
electron:mainfrom
felixrieseberg:felixr/remove-experimental-fetch-option

Conversation

@felixrieseberg

Copy link
Copy Markdown
Member

Description of Change

Node.js removed the experimental_fetch option in nodejs/node#62759 since fetch is now unconditionally enabled. We had been patching the field back in so that node_main.cc could read it and node_bindings.cc could pass --no-experimental-fetch to renderer/worker processes.

Neither is necessary:

  • In ELECTRON_RUN_AS_NODE mode, fetch is always available, so WASM streaming (which depends on Response) should always be set up.
  • In renderer/worker processes, kNoBrowserGlobals already causes Node's PrincipalRealm::BootstrapRealm to skip internal/bootstrap/web/exposed-window-or-worker.js, which is where fetch/Request/Response/Headers/FormData are defined. Node's fetch globals were never being installed; Blink's win by default.

This drops the patch and the two call sites that depended on it.

Closes #51912.

Note: I've used Claude to help me with writing and verifying this change. To verify, I've build & confirmed that we're using native code both in main and in a renderer with Node. A unit test seemed inappropriate, but to verify:

Details
$ ELECTRON_RUN_AS_NODE=1 e start -e 'console.log(fetch.toString().slice(0,60)); WebAssembly.compileStreaming(Promise.resolve("x")).catch(e=>console.log(e.constructor.name+":",e.message))'
function fetch(input, init = undefined) { // eslint-disable-
TypeError: The "source" argument must be an instance of Response or an Promise resolving to Response. Received type string ('x')

The TypeError originates from Node's internal/wasm_web_api.js, which only runs when SetWasmStreamingCallback has been installed on the isolate. Without it, V8 would throw WebAssembly streaming APIs are not supported.

Renderer with nodeIntegration ‚ I confirmed Blink's fetch is exposed (not Node's undici), via webContents.executeJavaScript:

const { app, BrowserWindow } = require('electron');
const fs = require('fs');
app.whenReady().then(async () => {
  const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, contextIsolation: false } });
  await w.loadURL('data:text/html,<html></html>');
  const r = await w.webContents.executeJavaScript(`({
    fetchSrc: fetch.toString(),
    fetchIsNative: fetch.toString().includes('[native code]'),
    ResponseIsNative: Response.toString().includes('[native code]'),
    HeadersIsNative: Headers.toString().includes('[native code]'),
    FormDataIsNative: FormData.toString().includes('[native code]'),
    RequestIsNative: Request.toString().includes('[native code]'),
    hasProcess: typeof process !== 'undefined',
    nodeVer: typeof process !== 'undefined' ? process.versions.node : null,
  })`);
  fs.writeFileSync(process.env.OUT_FILE, JSON.stringify(r, null, 2));
  app.exit(r.fetchIsNative && r.ResponseIsNative ? 0 : 1);
}).catch(e => { fs.writeFileSync(process.env.OUT_FILE, 'ERR: ' + e.stack); app.exit(2); });
{
  "fetchSrc": "function fetch() { [native code] }",
  "fetchIsNative": true,
  "ResponseIsNative": true,
  "HeadersIsNative": true,
  "FormDataIsNative": true,
  "RequestIsNative": true,
  "nodeVer": "24.16.0"
}

All five globals report [native code] (Blink). If Node's bootstrap had installed them, fetch.toString() would show the undici wrapper source instead.

Checklist

Release Notes

Notes: none

@felixrieseberg
felixrieseberg requested a review from a team as a code owner June 8, 2026 20:58
@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Jun 8, 2026
@felixrieseberg felixrieseberg added ai-pr PR that has been determined to be completely or majority untested and ai generated and removed ai-pr PR that has been determined to be completely or majority untested and ai generated labels Jun 8, 2026
@electron-issue-triage

Copy link
Copy Markdown

AI PR Detected

Hello @felixrieseberg. Due to the high amount of AI spam PRs we receive, if a PR is detected to be majority AI-generated without disclosure and untested, we will automatically close the PR.

We welcome the use of AI tools, as long as the PR meets our quality standards and has clearly been built and tested. If you believe your PR was closed in error, we welcome you to resubmit. However, please read our CONTRIBUTING.md and AI Tool Policy carefully before reopening. Thanks for your contribution.

@felixrieseberg

Copy link
Copy Markdown
Member Author

Whoeps, I should have not added the AI label 😅

@dsanders11 dsanders11 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A unit test seemed inappropriate

I think this PR should add test coverage in spec/node-spec.ts for node integration:

  • A simple check that WASM streaming works with fetch in the renderer
  • That fetch.toString() is [native code] as expected in the renderer

@dsanders11 dsanders11 added semver/patch backwards-compatible bug fixes target/43-x-y PR should also be added to the "43-x-y" branch. labels Jun 9, 2026
@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Jun 9, 2026
@felixrieseberg

felixrieseberg commented Jun 10, 2026

Copy link
Copy Markdown
Member Author

Tests added!

This PR now supersedes the Node-patch extension in #51953 by handling the callback restoration in Electron's renderer code instead; #51953 remains the right backportable fix for release branches. WASM streaming regression coverage lives in #51953's chromium-spec.ts test, so this PR only adds the [native code] assertion in node-spec.ts.

The blinkfetch/blinkResponse/‚ stash-and-restore dance in electron_renderer_client.cc, web_worker_observer.cc, lib/node/init.ts and lib/worker/init.ts is removed as dead code: under kNoBrowserGlobals Node neither installs nor deletes those globals, so Blink's bindings are never disturbed.

#51953 should land first

@dsanders11 dsanders11 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#51953 has landed and this PR needs to fix the merge conflict as a result.

The latest changes here also look to have introduced a failure of the "chromium features web workers Worker with nodeIntegrationInWorker has access to EventSource" test.

Node.js removed the experimental_fetch option in nodejs/node#62759
since fetch is now unconditionally enabled. We had been patching the
field back in so that node_main.cc could read it and node_bindings.cc
could pass --no-experimental-fetch to renderer/worker processes.

Neither is necessary:

- In ELECTRON_RUN_AS_NODE mode, fetch is always available, so WASM
  streaming (which depends on Response) should always be set up.
- In renderer/worker processes, kNoBrowserGlobals already causes
  Node's PrincipalRealm::BootstrapRealm to skip
  internal/bootstrap/web/exposed-window-or-worker.js, which is where
  fetch/Request/Response/Headers/FormData are defined. Node's fetch
  globals were never being installed; Blink's win by default.

Drop the patch and the two call sites that depended on it.

Closes electron#51912
SetIsolateUpForNode now unconditionally registers Node's WASM streaming
callback (nodejs/node#62759 dropped the experimental_fetch gate). With
kNoBrowserGlobals the JS half of that callback is never installed, so
WebAssembly.compileStreaming/instantiateStreaming would crash on
CHECK(\!impl.IsEmpty()) in node_wasm_web_api.cc.

Re-register Blink's handler via WasmResponseExtensions::Initialize after
CreateEnvironment in renderer and worker so Blink's implementation wins.

Also drop the now-redundant blink* global stash/restore: with
kNoBrowserGlobals Node neither installs nor deletes fetch/Response/etc,
so Blink's bindings are never disturbed and the round-trip through
blinkfetch was a no-op.

Add a spec asserting fetch/Headers/Request/Response/FormData stringify
to [native code] in a nodeIntegration renderer.
- Resolve modify/delete on the fetch patch (delete wins; the
  experimental_fetch gate it added to environment.cc is no longer
  needed since WasmResponseExtensions::Initialize handles the
  callback restoration in Electron's renderer code).
- Revert the environment.cc index hash in
  api_delete_deprecated_fields_on_v8_isolate.patch now that the
  preceding patch is gone (verified with `e patches node`).
- Preserve Blink's EventSource around LoadEnvironment in renderer
  and worker: Node's setupEventsource() in pre_execution.js deletes
  globalThis.EventSource whenever --experimental-eventsource is not
  set, and unlike setupNavigator/setupWebStorage it does not check
  noBrowserGlobals first. The previous blink* stash covered this as
  a side effect; restore just EventSource explicitly in C++.
- Extend the [native code] spec assertion to cover EventSource.
@felixrieseberg
felixrieseberg force-pushed the felixr/remove-experimental-fetch-option branch from ec7b623 to 69610d1 Compare June 14, 2026 16:49
The C++ save/restore around LoadEnvironment was too late: preload
scripts run inside LoadEnvironment, after Node's setupEventsource()
has deleted globalThis.EventSource but before the C++ restore. This
broke 'preload has synchronous access to all eventual window APIs'.

Pass --experimental-eventsource for renderer/worker so Node's
setupEventsource() becomes a no-op and Blink's binding is never
deleted. Under kNoBrowserGlobals Node never installs its own undici
EventSource, so the flag's only effect is suppressing the delete.
@jkleinsc
jkleinsc requested a review from dsanders11 June 16, 2026 20:33
@jkleinsc
jkleinsc merged commit 940b24d into electron:main Jun 16, 2026
71 checks passed
@release-clerk

release-clerk Bot commented Jun 16, 2026

Copy link
Copy Markdown

No Release Notes

@trop

trop Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

I have automatically backported this PR to "43-x-y", please check out #52028

@trop trop Bot added in-flight/43-x-y and removed target/43-x-y PR should also be added to the "43-x-y" branch. labels Jun 16, 2026
@trop trop Bot added merged/43-x-y PR was merged to the "43-x-y" branch. and removed in-flight/43-x-y labels Jun 18, 2026
mitchchn pushed a commit to mitchchn/electron that referenced this pull request Jun 19, 2026
…tron#51923)

* chore: remove EnvironmentOptions::experimental_fetch dependency

Node.js removed the experimental_fetch option in nodejs/node#62759
since fetch is now unconditionally enabled. We had been patching the
field back in so that node_main.cc could read it and node_bindings.cc
could pass --no-experimental-fetch to renderer/worker processes.

Neither is necessary:

- In ELECTRON_RUN_AS_NODE mode, fetch is always available, so WASM
  streaming (which depends on Response) should always be set up.
- In renderer/worker processes, kNoBrowserGlobals already causes
  Node's PrincipalRealm::BootstrapRealm to skip
  internal/bootstrap/web/exposed-window-or-worker.js, which is where
  fetch/Request/Response/Headers/FormData are defined. Node's fetch
  globals were never being installed; Blink's win by default.

Drop the patch and the two call sites that depended on it.

Closes electron#51912

* fix: restore Blink's WASM streaming callback in renderer/worker

SetIsolateUpForNode now unconditionally registers Node's WASM streaming
callback (nodejs/node#62759 dropped the experimental_fetch gate). With
kNoBrowserGlobals the JS half of that callback is never installed, so
WebAssembly.compileStreaming/instantiateStreaming would crash on
CHECK(\!impl.IsEmpty()) in node_wasm_web_api.cc.

Re-register Blink's handler via WasmResponseExtensions::Initialize after
CreateEnvironment in renderer and worker so Blink's implementation wins.

Also drop the now-redundant blink* global stash/restore: with
kNoBrowserGlobals Node neither installs nor deletes fetch/Response/etc,
so Blink's bindings are never disturbed and the round-trip through
blinkfetch was a no-op.

Add a spec asserting fetch/Headers/Request/Response/FormData stringify
to [native code] in a nodeIntegration renderer.

* fixup: rebase on electron#51953 and preserve Blink's EventSource

- Resolve modify/delete on the fetch patch (delete wins; the
  experimental_fetch gate it added to environment.cc is no longer
  needed since WasmResponseExtensions::Initialize handles the
  callback restoration in Electron's renderer code).
- Revert the environment.cc index hash in
  api_delete_deprecated_fields_on_v8_isolate.patch now that the
  preceding patch is gone (verified with `e patches node`).
- Preserve Blink's EventSource around LoadEnvironment in renderer
  and worker: Node's setupEventsource() in pre_execution.js deletes
  globalThis.EventSource whenever --experimental-eventsource is not
  set, and unlike setupNavigator/setupWebStorage it does not check
  noBrowserGlobals first. The previous blink* stash covered this as
  a side effect; restore just EventSource explicitly in C++.
- Extend the [native code] spec assertion to cover EventSource.

* fixup: pass --experimental-eventsource instead of C++ save/restore

The C++ save/restore around LoadEnvironment was too late: preload
scripts run inside LoadEnvironment, after Node's setupEventsource()
has deleted globalThis.EventSource but before the C++ restore. This
broke 'preload has synchronous access to all eventual window APIs'.

Pass --experimental-eventsource for renderer/worker so Node's
setupEventsource() becomes a no-op and Blink's binding is never
deleted. Under kNoBrowserGlobals Node never installs its own undici
EventSource, so the flag's only effect is suppressing the delete.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

44-x-y merged/43-x-y PR was merged to the "43-x-y" branch. semver/patch backwards-compatible bug fixes

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

remove EnvironmentOptions::experimental_fetch

4 participants