chore: remove EnvironmentOptions::experimental_fetch dependency#51923
Conversation
|
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. |
|
Whoeps, I should have not added the AI label 😅 |
dsanders11
left a comment
There was a problem hiding this comment.
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
fetchin the renderer - That
fetch.toString()is[native code]as expected in the renderer
|
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 The #51953 should land first |
dsanders11
left a comment
There was a problem hiding this comment.
#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.
ec7b623 to
69610d1
Compare
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.
|
No Release Notes |
|
I have automatically backported this PR to "43-x-y", please check out #52028 |
…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.
Description of Change
Node.js removed the
experimental_fetchoption in nodejs/node#62759 since fetch is now unconditionally enabled. We had been patching the field back in so thatnode_main.cccould read it andnode_bindings.cccould pass--no-experimental-fetchto renderer/worker processes.Neither is necessary:
ELECTRON_RUN_AS_NODEmode, fetch is always available, so WASM streaming (which depends onResponse) should always be set up.kNoBrowserGlobalsalready causes Node'sPrincipalRealm::BootstrapRealmto skipinternal/bootstrap/web/exposed-window-or-worker.js, which is wherefetch/Request/Response/Headers/FormDataare 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
The
TypeErrororiginates from Node'sinternal/wasm_web_api.js, which only runs whenSetWasmStreamingCallbackhas been installed on the isolate. Without it, V8 would throwWebAssembly streaming APIs are not supported.Renderer with
nodeIntegration‚ I confirmed Blink's fetch is exposed (not Node's undici), viawebContents.executeJavaScript:{ "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
npm testpassesRelease Notes
Notes: none