Skip to content

Remove node-fetch dependency and use built-in https/http modules instead#963

Open
Shooteger wants to merge 1 commit into
mapbox:masterfrom
Shooteger:master
Open

Remove node-fetch dependency and use built-in https/http modules instead#963
Shooteger wants to merge 1 commit into
mapbox:masterfrom
Shooteger:master

Conversation

@Shooteger
Copy link
Copy Markdown

This package here already requires node >= 18, so node-fetch is just dead weight so I replaced it with the built-in https/http modules in lib/install.js

Also fixed some bugs while doing this:

  • ca and cafile were read from opts but never actually passed to fetch() which was silently ignored before this PR
  • same for the User-Agent header, built but thrown away
  • follow_max: 10 in requestOpts was dead code, node-fetch never understood it,
    so redirect loops would just hang forever (its 20 I think, in node-fetch v2)
// ca is written into requestOpts...
// follow_max is set in requestOpts...
const requestOpts = {
  uri: sanitized,
  headers: { 'User-Agent': '...' },
  follow_max: 10  // node-fetch has never had this option
};

// ...but fetch() only receives { agent }, so follow_max is thrown away too.
// node-fetch's actual option is `follow`, not `follow_max`.
// Without it, node-fetch defaults to 20 redirects — not 10 as intended.
fetch(sanitized, { agent })

The new fetch_with_redirects() helper fixes all of that. It brings proper 10 redirect cap,
ca/agent/headers all wired through correctly, returns an IncomingMessage so
.pipe() into tar works directly.

native fetch in Node 18+ uses undici under the hood, not the http/https modules.
nock only intercepts http/https, so the whole test suite would break silently.

Also ran node scripts/abi_crosswalk.js and committed the changes

930 tests (all) passing.

Please review and let me know, if it can be merged

MP

Replaces node-fetch with node's built-in https/http modules in
lib/install.js. Since node >=18 is already required, the dependency
is unnecessary

Fixes three pre-existing bugs where ca, cafile, and the
User-Agent header were built into requestOpts but never actually
passed to fetch(), and where follow_max had no effect leaving
redirect loops unbounded.

- add fetch_with_redirects() helper with correct redirect cap (10)
- wire ca, cafile, and User-Agent through on every request
- export fetch_with_redirects for use in proxy-bcrypt test
- update test/proxy-bcrypt.test.js to drop node-fetch
- run node scripts/abi_crosswalk.js, update abi_crosswalk.json
@Shooteger Shooteger requested a review from a team as a code owner May 25, 2026 19:41
@Shooteger
Copy link
Copy Markdown
Author

For reference, in src/index.js for node-fetch 2.x, we have this:

// HTTP-redirect fetch step 6 (counter increment)
// Create a new Request object.
const requestOpts = {
    headers: new Headers(request.headers),
    follow: request.follow,  // <-- the option is called `follow`, not `follow_max`
    counter: request.counter + 1,
    agent: request.agent,
    ...
};

with counter

if (request.counter >= request.follow) {
    reject(new FetchError(`maximum redirect reached at: ${request.url}`, 'max-redirect'));

Sign up for free to 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.

1 participant