feat: replace make-fetch-happen with built-in fetch - #3302

Merged
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch
Apr 21, 2026
Merged

feat: replace make-fetch-happen with built-in fetch#3302
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch

Conversation

@MarshallOfSound

Copy link
Copy Markdown
Member

Swaps make-fetch-happen for 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.

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.
@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

The Lint Python error was fixed elsewhere.

@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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 😅

@cclauss

Copy link
Copy Markdown
Contributor

I like the goal. There is nothing to do on the Lint Python error, but the other GitHub Actions tests need to pass.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.js to built-in fetch, adding an undici dispatcher 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-happen dependency with undici.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

FileDescription
lib/download.jsImplements built-in fetch + EnvHttpProxyAgent, and adapts response handling for existing installer logic.
test/test-download.jsUpdates proxy tests to use CONNECT tunneling and verifies proxy usage/no-proxy behavior.
package.jsonDrops make-fetch-happen and adds undici@^6.25.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/download.js Outdated
Comment threadtest/test-download.js Outdated
Comment threadtest/test-download.js
- 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
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@cclauss Current CI failures are the same set fixed by #3300. I think this PR is in a good state

@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

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.
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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

@cclausscclauss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Can another maintainer please review these changes?

@cclauss
cclauss requested a review from gengjiawenApril 19, 2026 20:39
Comment threadlib/download.js
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.
@gengjiawen
gengjiawen merged commit 393ec2b into nodejs:mainApr 21, 2026
37 checks passed
@cclausscclauss mentioned this pull request Apr 21, 2026
2 tasks
hlovdal pushed a commit to hlovdal/node-gyp that referenced this pull request Jun 2, 2026
* feat: replace make-fetch-happen with built-in fetch
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.
* fixup: address review feedback
- 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
* ci: fix Python lint and npm install in tests workflow
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.
* fix: apply cafile to proxied TLS connections
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.
(cherry picked from commit 393ec2b)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MarshallOfSound@cclauss@gengjiawen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat: replace make-fetch-happen with built-in fetch - #3302

Merged
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch
Apr 21, 2026
Merged

feat: replace make-fetch-happen with built-in fetch#3302
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch

Conversation

@MarshallOfSound

Copy link
Copy Markdown
Member

Swaps make-fetch-happen for 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.

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.
@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

The Lint Python error was fixed elsewhere.

@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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 😅

@cclauss

Copy link
Copy Markdown
Contributor

I like the goal. There is nothing to do on the Lint Python error, but the other GitHub Actions tests need to pass.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.js to built-in fetch, adding an undici dispatcher 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-happen dependency with undici.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

FileDescription
lib/download.jsImplements built-in fetch + EnvHttpProxyAgent, and adapts response handling for existing installer logic.
test/test-download.jsUpdates proxy tests to use CONNECT tunneling and verifies proxy usage/no-proxy behavior.
package.jsonDrops make-fetch-happen and adds undici@^6.25.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/download.js Outdated
Comment threadtest/test-download.js Outdated
Comment threadtest/test-download.js
- 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
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@cclauss Current CI failures are the same set fixed by #3300. I think this PR is in a good state

@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

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.
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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

@cclausscclauss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Can another maintainer please review these changes?

@cclauss
cclauss requested a review from gengjiawenApril 19, 2026 20:39
Comment threadlib/download.js
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.
@gengjiawen
gengjiawen merged commit 393ec2b into nodejs:mainApr 21, 2026
37 checks passed
@cclausscclauss mentioned this pull request Apr 21, 2026
2 tasks
hlovdal pushed a commit to hlovdal/node-gyp that referenced this pull request Jun 2, 2026
* feat: replace make-fetch-happen with built-in fetch
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.
* fixup: address review feedback
- 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
* ci: fix Python lint and npm install in tests workflow
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.
* fix: apply cafile to proxied TLS connections
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.
(cherry picked from commit 393ec2b)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MarshallOfSound@cclauss@gengjiawen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat: replace make-fetch-happen with built-in fetch - #3302

Merged
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch
Apr 21, 2026
Merged

feat: replace make-fetch-happen with built-in fetch#3302
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch

Conversation

@MarshallOfSound

Copy link
Copy Markdown
Member

Swaps make-fetch-happen for 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.

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.
@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

The Lint Python error was fixed elsewhere.

@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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 😅

@cclauss

Copy link
Copy Markdown
Contributor

I like the goal. There is nothing to do on the Lint Python error, but the other GitHub Actions tests need to pass.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.js to built-in fetch, adding an undici dispatcher 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-happen dependency with undici.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

FileDescription
lib/download.jsImplements built-in fetch + EnvHttpProxyAgent, and adapts response handling for existing installer logic.
test/test-download.jsUpdates proxy tests to use CONNECT tunneling and verifies proxy usage/no-proxy behavior.
package.jsonDrops make-fetch-happen and adds undici@^6.25.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/download.js Outdated
Comment threadtest/test-download.js Outdated
Comment threadtest/test-download.js
- 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
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@cclauss Current CI failures are the same set fixed by #3300. I think this PR is in a good state

@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

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.
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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

@cclausscclauss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Can another maintainer please review these changes?

@cclauss
cclauss requested a review from gengjiawenApril 19, 2026 20:39
Comment threadlib/download.js
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.
@gengjiawen
gengjiawen merged commit 393ec2b into nodejs:mainApr 21, 2026
37 checks passed
@cclausscclauss mentioned this pull request Apr 21, 2026
2 tasks
hlovdal pushed a commit to hlovdal/node-gyp that referenced this pull request Jun 2, 2026
* feat: replace make-fetch-happen with built-in fetch
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.
* fixup: address review feedback
- 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
* ci: fix Python lint and npm install in tests workflow
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.
* fix: apply cafile to proxied TLS connections
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.
(cherry picked from commit 393ec2b)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MarshallOfSound@cclauss@gengjiawen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat: replace make-fetch-happen with built-in fetch - #3302

Merged
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch
Apr 21, 2026
Merged

feat: replace make-fetch-happen with built-in fetch#3302
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch

Conversation

@MarshallOfSound

Copy link
Copy Markdown
Member

Swaps make-fetch-happen for 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.

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.
@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

The Lint Python error was fixed elsewhere.

@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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 😅

@cclauss

Copy link
Copy Markdown
Contributor

I like the goal. There is nothing to do on the Lint Python error, but the other GitHub Actions tests need to pass.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.js to built-in fetch, adding an undici dispatcher 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-happen dependency with undici.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

FileDescription
lib/download.jsImplements built-in fetch + EnvHttpProxyAgent, and adapts response handling for existing installer logic.
test/test-download.jsUpdates proxy tests to use CONNECT tunneling and verifies proxy usage/no-proxy behavior.
package.jsonDrops make-fetch-happen and adds undici@^6.25.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/download.js Outdated
Comment threadtest/test-download.js Outdated
Comment threadtest/test-download.js
- 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
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@cclauss Current CI failures are the same set fixed by #3300. I think this PR is in a good state

@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

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.
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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

@cclausscclauss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Can another maintainer please review these changes?

@cclauss
cclauss requested a review from gengjiawenApril 19, 2026 20:39
Comment threadlib/download.js
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.
@gengjiawen
gengjiawen merged commit 393ec2b into nodejs:mainApr 21, 2026
37 checks passed
@cclausscclauss mentioned this pull request Apr 21, 2026
2 tasks
hlovdal pushed a commit to hlovdal/node-gyp that referenced this pull request Jun 2, 2026
* feat: replace make-fetch-happen with built-in fetch
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.
* fixup: address review feedback
- 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
* ci: fix Python lint and npm install in tests workflow
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.
* fix: apply cafile to proxied TLS connections
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.
(cherry picked from commit 393ec2b)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MarshallOfSound@cclauss@gengjiawen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat: replace make-fetch-happen with built-in fetch - #3302

Merged
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch
Apr 21, 2026
Merged

feat: replace make-fetch-happen with built-in fetch#3302
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch

Conversation

@MarshallOfSound

Copy link
Copy Markdown
Member

Swaps make-fetch-happen for 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.

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.
@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

The Lint Python error was fixed elsewhere.

@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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 😅

@cclauss

Copy link
Copy Markdown
Contributor

I like the goal. There is nothing to do on the Lint Python error, but the other GitHub Actions tests need to pass.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.js to built-in fetch, adding an undici dispatcher 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-happen dependency with undici.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

FileDescription
lib/download.jsImplements built-in fetch + EnvHttpProxyAgent, and adapts response handling for existing installer logic.
test/test-download.jsUpdates proxy tests to use CONNECT tunneling and verifies proxy usage/no-proxy behavior.
package.jsonDrops make-fetch-happen and adds undici@^6.25.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/download.js Outdated
Comment threadtest/test-download.js Outdated
Comment threadtest/test-download.js
- 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
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@cclauss Current CI failures are the same set fixed by #3300. I think this PR is in a good state

@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

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.
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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

@cclausscclauss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Can another maintainer please review these changes?

@cclauss
cclauss requested a review from gengjiawenApril 19, 2026 20:39
Comment threadlib/download.js
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.
@gengjiawen
gengjiawen merged commit 393ec2b into nodejs:mainApr 21, 2026
37 checks passed
@cclausscclauss mentioned this pull request Apr 21, 2026
2 tasks
hlovdal pushed a commit to hlovdal/node-gyp that referenced this pull request Jun 2, 2026
* feat: replace make-fetch-happen with built-in fetch
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.
* fixup: address review feedback
- 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
* ci: fix Python lint and npm install in tests workflow
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.
* fix: apply cafile to proxied TLS connections
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.
(cherry picked from commit 393ec2b)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MarshallOfSound@cclauss@gengjiawen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat: replace make-fetch-happen with built-in fetch - #3302

Merged
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch
Apr 21, 2026
Merged

feat: replace make-fetch-happen with built-in fetch#3302
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch

Conversation

@MarshallOfSound

Copy link
Copy Markdown
Member

Swaps make-fetch-happen for 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.

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.
@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

The Lint Python error was fixed elsewhere.

@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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 😅

@cclauss

Copy link
Copy Markdown
Contributor

I like the goal. There is nothing to do on the Lint Python error, but the other GitHub Actions tests need to pass.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.js to built-in fetch, adding an undici dispatcher 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-happen dependency with undici.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

FileDescription
lib/download.jsImplements built-in fetch + EnvHttpProxyAgent, and adapts response handling for existing installer logic.
test/test-download.jsUpdates proxy tests to use CONNECT tunneling and verifies proxy usage/no-proxy behavior.
package.jsonDrops make-fetch-happen and adds undici@^6.25.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/download.js Outdated
Comment threadtest/test-download.js Outdated
Comment threadtest/test-download.js
- 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
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@cclauss Current CI failures are the same set fixed by #3300. I think this PR is in a good state

@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

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.
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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

@cclausscclauss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Can another maintainer please review these changes?

@cclauss
cclauss requested a review from gengjiawenApril 19, 2026 20:39
Comment threadlib/download.js
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.
@gengjiawen
gengjiawen merged commit 393ec2b into nodejs:mainApr 21, 2026
37 checks passed
@cclausscclauss mentioned this pull request Apr 21, 2026
2 tasks
hlovdal pushed a commit to hlovdal/node-gyp that referenced this pull request Jun 2, 2026
* feat: replace make-fetch-happen with built-in fetch
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.
* fixup: address review feedback
- 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
* ci: fix Python lint and npm install in tests workflow
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.
* fix: apply cafile to proxied TLS connections
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.
(cherry picked from commit 393ec2b)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MarshallOfSound@cclauss@gengjiawen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat: replace make-fetch-happen with built-in fetch - #3302

Merged
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch
Apr 21, 2026
Merged

feat: replace make-fetch-happen with built-in fetch#3302
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch

Conversation

@MarshallOfSound

Copy link
Copy Markdown
Member

Swaps make-fetch-happen for 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.

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.
@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

The Lint Python error was fixed elsewhere.

@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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 😅

@cclauss

Copy link
Copy Markdown
Contributor

I like the goal. There is nothing to do on the Lint Python error, but the other GitHub Actions tests need to pass.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.js to built-in fetch, adding an undici dispatcher 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-happen dependency with undici.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

FileDescription
lib/download.jsImplements built-in fetch + EnvHttpProxyAgent, and adapts response handling for existing installer logic.
test/test-download.jsUpdates proxy tests to use CONNECT tunneling and verifies proxy usage/no-proxy behavior.
package.jsonDrops make-fetch-happen and adds undici@^6.25.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/download.js Outdated
Comment threadtest/test-download.js Outdated
Comment threadtest/test-download.js
- 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
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@cclauss Current CI failures are the same set fixed by #3300. I think this PR is in a good state

@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

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.
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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

@cclausscclauss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Can another maintainer please review these changes?

@cclauss
cclauss requested a review from gengjiawenApril 19, 2026 20:39
Comment threadlib/download.js
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.
@gengjiawen
gengjiawen merged commit 393ec2b into nodejs:mainApr 21, 2026
37 checks passed
@cclausscclauss mentioned this pull request Apr 21, 2026
2 tasks
hlovdal pushed a commit to hlovdal/node-gyp that referenced this pull request Jun 2, 2026
* feat: replace make-fetch-happen with built-in fetch
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.
* fixup: address review feedback
- 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
* ci: fix Python lint and npm install in tests workflow
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.
* fix: apply cafile to proxied TLS connections
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.
(cherry picked from commit 393ec2b)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MarshallOfSound@cclauss@gengjiawen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

feat: replace make-fetch-happen with built-in fetch - #3302

Merged
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch
Apr 21, 2026
Merged

feat: replace make-fetch-happen with built-in fetch#3302
gengjiawen merged 4 commits into
nodejs:mainfrom
MarshallOfSound:sam/built-in-fetch

Conversation

@MarshallOfSound

Copy link
Copy Markdown
Member

Swaps make-fetch-happen for 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.

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.
@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

The Lint Python error was fixed elsewhere.

@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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 😅

@cclauss

Copy link
Copy Markdown
Contributor

I like the goal. There is nothing to do on the Lint Python error, but the other GitHub Actions tests need to pass.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.js to built-in fetch, adding an undici dispatcher 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-happen dependency with undici.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

FileDescription
lib/download.jsImplements built-in fetch + EnvHttpProxyAgent, and adapts response handling for existing installer logic.
test/test-download.jsUpdates proxy tests to use CONNECT tunneling and verifies proxy usage/no-proxy behavior.
package.jsonDrops make-fetch-happen and adds undici@^6.25.0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/download.js Outdated
Comment threadtest/test-download.js Outdated
Comment threadtest/test-download.js
- 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
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@cclauss Current CI failures are the same set fixed by #3300. I think this PR is in a good state

@cclauss

cclauss commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

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.
@MarshallOfSound

Copy link
Copy Markdown
MemberAuthor

@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

@cclausscclauss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Can another maintainer please review these changes?

@cclauss
cclauss requested a review from gengjiawenApril 19, 2026 20:39
Comment threadlib/download.js
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.
@gengjiawen
gengjiawen merged commit 393ec2b into nodejs:mainApr 21, 2026
37 checks passed
@cclausscclauss mentioned this pull request Apr 21, 2026
2 tasks
hlovdal pushed a commit to hlovdal/node-gyp that referenced this pull request Jun 2, 2026
* feat: replace make-fetch-happen with built-in fetch
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.
* fixup: address review feedback
- 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
* ci: fix Python lint and npm install in tests workflow
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.
* fix: apply cafile to proxied TLS connections
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.
(cherry picked from commit 393ec2b)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MarshallOfSound@cclauss@gengjiawen