New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(adapters): add fetch
adapter
#5146
Conversation
@jasonsaayman unable to assign, but early feedback would be welcome (along with a CI run). i'm going to keep adding tests etc. and refining the implementation with an eye toward (1) parity with XHR and |
@jasonsaayman nearly done with the testsuite here and will rebase/push up soon. could i get a review? wdyt of the approach? |
@sgammon great to see this PR! 😍 do you think I should use your branch in a project already for testing? We have a project using axios but now use service workers and require the fetch adapter. we're happy to test and give feedback if needed. |
@bumi you are welcome to give it a try, but there are multiple Axios features that aren't supported yet (most notably, download and upload events). i would love feedback of course and can quickly patch any issues you encounter. |
a0de218
to
2379f3a
Compare
rebased and |
@sgammon this is looking awesome, I have approved the CI run and I will review this tonight in its entirety but prelim it looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
Gave it a try by using your fork/branch directly. |
@jasonsaayman thank you for the feedback and the CI run helps a ton. i'm slightly concerned about being more invasive in the testsuite, but I'll try to separate that change out in a commit so we can review. @escapedcat gotcha. thanks for letting me know, i'll take a look and cover that with a test. are you using the fetch adapter from node or the browser? |
we're running it in the browser. |
@bumi did you encounter an error as well? |
it would help to know if you guys are seeing errors, and if so, if they are surfacing via GET, POST, etc. browser tests will take a second because it depends on a slight refactor to the tests. |
bumi was speaking for "us" :) Yes, in the browser. |
@escapedcat ah understood! thank you :) |
Any ETA when this will land @sgammon ? |
@baraeb92 i'm about halfway done with the testsuite refactor, so i would estimate a week or less. |
This seems to be related to the way I tried to make this work. Imported it like this:
But this might fail because of the build-setup. Don't mind my comment. Looking forward to give this a try once it's merged. |
cool, wait :) |
thank you guys, for your patience here. it's taking a while because this refactor is large and, in order to maximally guarantee a merge at the end of this work, it needs quite a bit of cleanup to keep the change minimal. i'll have a new version to test shortly here |
@sgammon how do I best test it and import and configure the fetch adapter in a project (using your branch as a dependency?) I can use it and test it in a a few projects if that's helpfu. |
2379f3a
to
61b85e8
Compare
hey everybody, thank you for your attention on this PR. i've pushed an updated branch, with several bugs fixed, several features landed, and so on. i'll summarize in the PR summary up top and provide testing instructions for Browser and Node environments as well. |
@jasonsaayman can I get a CI run? |
- Instead of failing, bypass URL parsing, and pass the URL verbatim to the underlying fetch implementation
- Move abstract fetch testsuite tests into basic test spec file
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Relates to axios#5146 Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
9b315e1
to
7b83ff9
Compare
Hey everyone, I've backed up this branch, rebased it against I'll look into a Testsuites
|
@jasonsaayman This is now rebased and clean on top of Now that we're rebased on top of It would be cool to get workflow approval in the meantime so I can make sure CI is green against the main repo. |
Thank you for pushing this fix forward as it bet this will help a lot of people dealing with this issue for so long and using old versions of axios + 3rd party adapters! ❤️ |
Just wanted to say thanks so much for working on this. It's going to be a game changer when wanting to use API clients in a serverless environment. So many of them are unusable because of the axios dependency. Currently wanting to use |
WTF ?! Someone just out of the blue dumped 1015 additions and 127 deletions in a single commit implementing a |
Ah well, it is what it is. I tagged @DigitalBrainJS and @jasonsaayman several times but nobody ever got back to me. Ultimately I can see why they started fresh: it's probably best to develop this off My testsuite refactor is a good thing (it means the testsuite can be used generically against each adapter), but it necessarily means a big PR (or multiple PRs), and I understand the maintainers not wanting to approach that way. Ultimately I'm just happy Axios is actually so influential that arguably Thank you everyone for your help getting this across the line, with testing, feedback, and everything else. |
This is weird... |
@KaKi87 Well, not everything that is being worked on is done publicly, at least that which requires intensive research work, when the code changes too intensively to commit it remotely with a reasonable commit history and the code is scattered across dozens of branches to test approaches, hacks, and usability. @sgammon Thanks for your efforts. Without a doubt, this is a great job. 🤘 I won’t say that it won’t be possible to integrate this in principle, but it definitely can’t be published in Axios v1. Perhaps we can return to this in the next major version, for now, we are only talking about version v1. The future major roadmap is not yet clear. |
@DigitalBrainJS No worries, I appreciate your note a lot and the position you are in maintaining such an important project. I'll reach out over email and maybe connect with you on Discord to hand over lessons learned and some convos with standards bodies that might be helpful to you. |
It should, that's what openness is.
Which the community could have contributed to.
At the very very least, the progress of this work should have been shared as updates to the issue even if just in the form of announcement if you wouldn't want collaboration.
This feedback should have been brought during the early development of the PR so that the author would have had a chance to act on it. Or, if the private work was already going at that time, then this PR's author wouldn't have lost time authoring it if they knew as per an announcement that the work was already going on. |
I had been following this PR for a long time, and then one day, the repository maintainer completed the feature themselves. This shows that this repository does not welcome open source contributions, or even ignores them. https://github.com/axios/axios/blob/main/CONTRIBUTING.md?plain=1#L3 funny. |
Note: This is an evolving draft which does not yet support several Axios features.
Summary
This changeset adds a
fetch
-based adapter implementation toaxios
, powered bycross-fetch
and a native Fetch API adapter. The implementation may thus be used in Node, Browser, and pure-JS environments which support the Fetch API Specification.Please see the Implementation notes below for PR review, or if you are curious and wish to test.
Further reading:
cross-fetch
on NPMfetch
docs on MDNHow to use it (examples)
Latest version:
1.2.1-fetch-beta5
Use it in a browser
The browser version is available at a test URL, as is the ESM version. The complete samples for the below code snippets are available in
examples/fetch/browser
:Browser (ESM)
Browser (non-ESM)
Use it from Node
The Node distribution should be installable directly from this Git branch, like shown below.
Run it with
node <your-script>.js
to see:Use it from Deno
You can pull right from GitHub, or use the new
generic
distribution published to the test URL.Use it from CloudFlare Workers (new!)
Use the new
generic
bundle as an ESM import.Module-style workers:
Event-style workers:
Note: Live CloudFlare Worker example
Note: The
axios.elide.dev
URLs in this PR, for instance for the browser samples, all use a CloudFlare Worker that is implemented with Axios. So it is already in live testing and you can hit a live endpoint to see it in action simply by trying out the ESM browser examples (ESM modules require CORS, so the sample atexamples/fetch/workers
fetches the library from storage via Axios, adds CORS headers, and serves it).Beta release notes
Internal fixes for selection of handlers. Additional fixes for issues with source maps and typings. Build and test refactor.Beta 5
Fixes for compatibility with CloudFlare Workers, `generic` bundle now omits XHR adapter (**8kb compressed!**) and issues were fixed with the serving / reference of types and source maps.Beta 4
Several bugfixes related to response body decoding/handler selection. Support for CloudFlare Workers. Configuration docs and example docs for each platform.Beta 3
Updates new `generic` bundle to omit XHR. Makes set of "known" adapters (built-in adapters) variable per platform to allow for shipping bundles without XHR.Beta 2
Initial test version for browsers, generic targets (Deno, Bun, etc), and Node. Test instructions embedded in PR. Supports basic fetch operations with the new fetch adapter, URL `axios()` inputs.Beta 1
Known issues
Bug with selection of handler for text responses(fixed inbeta3
)Incompatibility with CloudFlare Workers(fixed inbeta3
)Generic bundle still ships with XHR(fixed inbeta4
)Further issues with text responses(fixed inbeta4
)Issues with source maps and typings(fixed inbeta5
)Changelog
fetch
adapter implementationfetch
to standard built-in adaptersgeneric
bundle target for pure JS environments (Deno, Workers, etc)generic
target without XHRgeneric
bundle weighs in at only ~8kb when compressed!URL
objectsurl
property support forURL
objectsfetchOptions
,fetcher
,parsedUrl
){adapter: 'fetch'}
) or reference ({adapter: axios.FetchAdapter}
)json
: Decode JSON responsestext
: Decode plain text responsesblob
: Provide response data in aBlob
document
: Provide HTML content in adocument
arraybuffer
: Raw data from an array bufferstream
: Pump raw data from a streamAbortSignal
)CancelToken
utf-8
fetch
Fixes and closes #1219, #4695, and supersedes #2891.
Implementation notes (click to expand)
This PR implements a Fetch API adapter for Axios, and updates various Axios interfaces to be compatible with objects provided by the Fetch API (
URL
,Request
, and so on). This PR is a work in progress, but feedback is welcome on all platforms supported by the Fetch API (Node, Browser, Workers, and anything else!).Fetch API integration desires are a moving target, particularly across platforms. For example, many developers want support for a Fetch adapter in the underlying implementation, which allows them to use Axios in more places (i.e. Cloudflare Workers). Some developers also want Fetch API integration with Axios itself, not just the adapter; for example, the ability to fetch
URL
andRequest
objects.Because this PR attempts to balance these concerns with backwards compatibility, the changes proposed are broken up into two clear categories: Inert changes, i.e., changes which are probably not controversial or breaking, and Invasive changes, which should be considered carefully before merge.
General notes
cross-fetch
library to ensure cross-platform interopwhatwg-fetch
polyfillDist
builds, to help with diagnosis during testing (this is temporary)Inert changes
Changes in this category are not expected to be controversial, because they are designed to work on all platforms supported by Axios (and then some), and use a well-supported extension path (adapters). Features in this section are (1) backward compatible, (2) entirely new code, and (3) inert unless activated explicitly by the user, at least until such time as the adapter is made a default option.
config
accepts a newfetchOptions
property which hands options directly to the underlyingfetch
implementationconfig
accepts a newfetcher
property which overrides the nativefetch
implementation, if anyInvasive (but backward-compatible) changes
I expect reasonable minds may differ about these changes. Since the Fetch API is available on many platforms and is gaining ground as a universal standard, it would be great if Axios’ integration with the Fetch API did more than just implement an adapter based on fetch; for example, the ability to
axios(URL(…))
oraxios(Request(…))
. Although this is a more seamless experience for consumers of Axios, it also represents a more invasive internal change (which can complicate testing, etc).Obviously, care is judiciously exercised to make sure these changes are fully backward-compatible with existing Axios constructor invocations, but that relies on the assumption that users are not already calling the constructor with monkey-patched fixes or other workarounds; i.e., there is a non-zero risk that these changes may break existing users, but those users would already be outside the supported path anyway.
axios
constructor now accepts aURL
objectaxios
config now accepts aURL
object in theurl
propertyaxios
config now supports aparsedUrl
property, for caching a pre-existingURL
objectaxios
constructor now accepts aRequest
objectIn cases where
axios
interfaces accept new objects that then need to be translated to Axios objects (namely, aRequest
or aResponse
), there will be a well-defined protocol and set of tests to make sure these changes work as intuitively as possible. For example, copyingHeaders
from one object to another, or the request method, etc.Unrelated changes
Several dependencies were dropped, updated, or otherwise changed in order to avoid vulnerabilities and update the toolchain for Axios. This was made possible by a change in CI: by building only against the latest "baseline" Node version (v18.x at the time of this writing), we can use
nvm
to test against each version without repeating the build.Additionally, testing was added for modern browsers in pure ESM via
web-test-runner
. This also fixes and clarifies library coverage which stands at 69%+ with the new adapter code included.Bundle / build stats
Bundle sizes:
Coming soon.
CI improvements:
Coming soon.