refactor: switch to native fetch implementation #5811
Merged
Conversation
This file contains 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
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
force-pushed
the
nflaig/native-fetch
branch
from
cd00212
to
cb4e403
Compare
|
Just mentioned in EL offline PR (#5797), we might wanna cover all error codes used here lodestar/packages/beacon-node/src/execution/engine/utils.ts Lines 39 to 40 in 59ec5fe I can try find out when those are thrown and add them in this PR |
|
After looking at further error scenarios outlined here (https://betterstack.com/community/guides/scaling-nodejs/nodejs-errors/) and in Node.js docs (https://nodejs.org/api/errors.html) I think the test coverage is sufficient. The underlying http client implementation is still the same, fetch is just a higher level abstraction. These errors are hard to test for locally / in tests:
It might be good to add |
|
🎉 This PR is included in v1.10.0 🎉 |
matthewkeil
added a commit
that referenced
this pull request
Aug 9, 2023
This reverts commit 3e65be7.
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.
Motivation
With recent issues we faced using cross-fetch (#5765 and
Failed to execute 'fetch' on 'Window': Illegal invocation) I would suggest we get rid of it and use native fetch instead as technically there is no reason not to do it.We already dropped support for node versions below 18.15.0 and native fetch is available on later versions and has to be explicitly be disabled by setting
--no-experimental-fetchflag.Native fetch in node is still Stability: 1 but based on the requirement for it to be compatible with the browser implementation I don't expect there will be breaking changes in the future.
Advantages
Disadvantages
Errors are not that great as details are inside
causeproperty which itself is of typeError. The errors thrown by node-fetch are much better for us as they have their own error handling which is different from native fetch in browser.The error message is not detailed at all, it just says
fetch failed, no context at all unless you look into causeTypeError: fetch failed at Object.fetch (node:internal/deps/undici/undici:11576:11) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) at async file:///home/nico/projects/test/fetch-test/addr-not-found.js:2:3 { cause: Error: getaddrinfo ENOTFOUND non-existant-domain.com at GetAddrInfoReqWrap.onlookupall [as oncomplete] (node:dns:118:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'non-existant-domain.com' } }Description
Removes cross-fetch and uses native fetch implementation instead. Inconsistent errors and intransparent messages are addressed by wrapping the native fetch implementation and throwing a custom
FetchError, similar to node-fetch error handling.Details about error handling can be found here
lodestar/packages/api/src/utils/client/fetch.ts
Line 24 in 33a0ceb
Compatibility
See Fetch_API#browser_compatibility for more details.
Closes #5765