feat: replace make-fetch-happen with built-in fetch#3302
feat: replace make-fetch-happen with built-in fetch#3302MarshallOfSound wants to merge 4 commits intonodejs:mainfrom
Conversation
Use Node's built-in fetch for downloading headers/tarballs, with undici's EnvHttpProxyAgent providing --proxy, --noproxy and --cafile support (plus http_proxy/https_proxy/no_proxy env var handling). Drops 36 transitive dependencies.
|
The Lint Python error was fixed elsewhere. |
|
@cclauss Any action from me? Pretty sure this is fully rebased. For context on this change, I'm just going on a mission trying to shrink the blast radius of Electron's core dependency tree. Supply Chain Risk mitigation and all that 😅 |
|
I like the goal. There is nothing to do on the |
There was a problem hiding this comment.
Pull request overview
This PR replaces make-fetch-happen with Node’s built-in fetch and uses undici’s EnvHttpProxyAgent to preserve proxy and custom CA behavior, reducing transitive dependencies and updating proxy-related test fixtures accordingly.
Changes:
- Switch
lib/download.jsto built-infetch, adding anundicidispatcher for proxy/CA support and adapting the returned response shape to existing call sites. - Update proxy tests to validate CONNECT-tunneling behavior and assert whether the proxy was actually used.
- Replace the
make-fetch-happendependency withundici.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/download.js |
Implements built-in fetch + EnvHttpProxyAgent, and adapts response handling for existing installer logic. |
test/test-download.js |
Updates proxy tests to use CONNECT tunneling and verifies proxy usage/no-proxy behavior. |
package.json |
Drops make-fetch-happen and adds undici@^6.25.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Guard Readable.fromWeb against null body (204/304 responses) - Add socket error handlers to CONNECT tunnel in proxy test - Destroy socket in noproxy test's CONNECT handler so a regression fails fast instead of hanging
|
Please fix the Python Lint issue with the f-string solution done in: Please fix the npm issue with the solution in |
Apply f-string fix from gyp-next#337 to resolve ruff F507 in simple_copy.py, and add npm@~11.10.0 pre-install step from nodejs#3300 to work around npm/cli#9151.
|
@cclauss Took those two isolated fixes into this branch as well so things should go green here 🙇 The gyp-next one will get clobbered on the next gyp-next sync but i guess that's fine the underlying fix will be in that sync too |
cclauss
left a comment
cclauss
left a comment
There was a problem hiding this comment.
LGTM. Can another maintainer please review these changes?
EnvHttpProxyAgent forwards opts to an internal ProxyAgent for proxied requests, which reads origin TLS config from requestTls rather than connect. Set connect/requestTls/proxyTls so the custom CA is honored on both direct and proxied paths. Adds a test covering https origin behind an HTTP CONNECT proxy with a custom CA.
Swaps
make-fetch-happenfor Node's built-in fetch, using undici's EnvHttpProxyAgent to keep --proxy / --noproxy / --cafile and the http_proxy / https_proxy / no_proxy env vars working. Drops 36 transitive dependencies.This results in a ~60+% drop in module "weight" for node-gyp, this one dependency spun out into a bunch of transitive dependencies that we just don't need in node >= 20.
Pinned to undici@^6 because 7.x requires Node >=20.18.1 and we still support 20.17.0. The dispatcher uses CONNECT tunneling for both http and https targets (vs absolute-URI for http previously), real proxies handle this fine, just meant the test fixture needed updating.