Remove node-fetch dependency and use built-in https/http modules instead#963
Open
Shooteger wants to merge 1 commit into
Open
Remove node-fetch dependency and use built-in https/http modules instead#963Shooteger wants to merge 1 commit into
Shooteger wants to merge 1 commit into
Conversation
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
Author
|
For reference, in src/index.js for node-fetch 2.x, we have this: with counter |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
so redirect loops would just hang forever (its 20 I think, in node-fetch v2)
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.jsand committed the changes930 tests (all) passing.
Please review and let me know, if it can be merged
MP