Skip to content
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

Closed
wants to merge 77 commits into from
Closed

Conversation

sgammon
Copy link

@sgammon sgammon commented Oct 17, 2022

Note: This is an evolving draft which does not yet support several Axios features.


Summary

This changeset adds a fetch-based adapter implementation to axios, powered by cross-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:


How 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)

Full sample available at ./examples/fetch/browser/minimal.html

<script type="module">
  import axios from "https://axios.elide.dev/axios/1.2.1-fetch-beta5/generic/axios.min.mjs";
  const response = await axios.get("https://httpbin.org/json", {adapter: 'fetch'});
  console.log("data from response: ", JSON.stringify(response.data));
</script>

Browser (non-ESM)

Full sample available at ./examples/fetch/browser/minimal.html

<script src="https://axios.elide.dev/axios/1.2.1-fetch-beta5/axios.min.js"></script>
<script>
  axios.get("https://httpbin.org/json", {adapter: 'fetch'}).then(function (response) {
    console.log("data from response: ", JSON.stringify(response.data));
  });
</script>
Use it from Node

The Node distribution should be installable directly from this Git branch, like shown below.

Full sample available at ./examples/fetch/node/axios-node-sample.js

// for temporary testing, before the feature is merged, you can do:
// `npm install axios@github:sgammon --ignore-scripts`

import axios from "axios";

function doTest(cbk, err) {
    axios.get("https://httpbin.org/json", {adapter: 'fetch'}).then(cbk, err);
}

doTest(function (response) {
    console.log("data from httpbin.org: ", response.data);
}, function (err) {
    throw err;
});

Run it with node <your-script>.js to see:

data from httpbin.org:  {
  slideshow: {
    author: 'Yours Truly',
    date: 'date of publication',
    slides: [ [Object], [Object] ],
    title: 'Sample Slide Show'
  }
}
Use it from Deno

You can pull right from GitHub, or use the new generic distribution published to the test URL.

Full sample available at ./examples/fetch/deno/axios-deno-sample.js

import axios from "https://axios.elide.dev/axios/1.2.1-fetch-beta5/generic/axios.mjs";
import {assertExists} from "https://deno.land/std@0.168.0/testing/asserts.ts";

Deno.test("fetch with axios on deno", async (t) => {
  const response = await axios.get("https://httpbin.org/json");
  assertExists(response, "should get a response")
});
Use it from CloudFlare Workers (new!)

Use the new generic bundle as an ESM import.

Full sample available at ./examples/fetch/workers/axios-worker.mjs

Module-style workers:

import axios from "axios/generic";

async function handler(request) {
  // use axios as normal
  return await axios({ /* ... */ }.then(response => {
    // make sure to rewrite into a Fetch response if you're returning directly
    return new Response(response.data, {
        status: response.status,
        statusText: response.statusText,
        headers: applyCorsHeaders(response.headers),
    });
  });
}

export default {
    async fetch(request) {
        return await handler(request);
    },
};

Event-style workers:

import axios from "axios/generic";

async function handler(request) {
  // use axios as normal
  return await axios({ /* ... */ }.then(response => {
    // make sure to rewrite into a Fetch response if you're returning directly
    return new Response(response.data, {
        status: response.status,
        statusText: response.statusText,
        headers: applyCorsHeaders(response.headers),
    });
  });
}

addEventListener("fetch", event => {
  event.respondWith(handler(event.request))
})

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 at examples/fetch/workers fetches the library from storage via Axios, adds CORS headers, and serves it).


Beta release notes

Beta 5

Internal fixes for selection of handlers. Additional fixes for issues with source maps and typings. Build and test refactor.

Beta 4

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 3

Several bugfixes related to response body decoding/handler selection. Support for CloudFlare Workers. Configuration docs and example docs for each platform.

Beta 2

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 1

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.

Known issues

  • None at this time (will update here)
  • Coverage of adapter not yet known
  • Unsupported features (cancellation, upload/download progress, streams, compression)
  • There are probably issues
  • Bug with selection of handler for text responses (fixed in beta3)
  • Incompatibility with CloudFlare Workers (fixed in beta3)
  • Generic bundle still ships with XHR (fixed in beta4)
  • Further issues with text responses (fixed in beta4)
  • Issues with source maps and typings (fixed in beta5)

Changelog

  • Primary adapter functionality
    • Add fetch adapter implementation
    • Add fetch to standard built-in adapters
    • Add generic bundle target for pure JS environments (Deno, Workers, etc)
    • Safely ship generic target without XHR
    • Bundle size: generic bundle weighs in at only ~8kb when compressed!
    • Axios API changes for Fetch API objects
      • Axios constructor support for URL objects
      • Axios config url property support for URL objects
      • Extensions to Axios config for Fetch adapter (fetchOptions, fetcher, parsedUrl)
      • Fetch adapter available by name ({adapter: 'fetch'}) or reference ({adapter: axios.FetchAdapter})
      • Utilities to convert between Fetch and Axios objects
    • Typings for new bundle
      • Module typings
      • CJS plain typings
      • Typings and source maps for distributions
    • Response type handlers
      • json: Decode JSON responses
      • text: Decode plain text responses
      • blob: Provide response data in a Blob
      • document: Provide HTML content in a document
      • arraybuffer: Raw data from an array buffer
      • stream: Pump raw data from a stream
    • In-flight request control
      • Request cancellation (AbortSignal)
      • Upload/download progress events
      • Support for CancelToken
    • Compression support
    • Encodings aside from utf-8
    • Maximum content length enforcement
    • Timeout enforcement
    • Full support for all Axios configurations
  • Testsuite for fetch
    • Add initial Node-side tests for fetch
    • Refactor to tests to use common setup / teardown code
    • Browser-side testing (initial tests)
    • Pure ESM tests
    • Full test coverage of Fetch adapter
    • Full TCK+testsuite complete
      • Browser (Standard)
      • Browser (ESM)
      • Node
      • Deno
  • Documentation updates
    • Document new adapter
    • Document new config properties
    • Corresponding site/docs PRs
    • Maybe an example of mocking, for easy testing?
    • Copy behavior
  • Examples for use with Fetch
    • Browser (JS & ESM)
    • Deno
    • Node
    • CloudFlare Workers
  • Corresponding Updates to CI
    • Test on Deno (experimental)
    • Test for pure JS (Workers, Bun)
    • Modern browser testing (ESM)
    • Coverage reporting

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 and Request 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

  • Uses same core fetch code for all environments, targeted at Fetch API spec
  • Uses cross-fetch library to ensure cross-platform interop
  • Compatible with whatwg-fetch polyfill
  • Fetch adapter debug logging is active, even in Dist builds, to help with diagnosis during testing (this is temporary)
  • Once ready to be merged, this PR will be rebased and squashed for clean / linear git history

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.

  • Implementation of the Fetch Adapter for Axios
  • Browser-side test suite for fetch adapter
  • Node-side test suite for fetch adapter
  • Axios config accepts a new fetchOptions property which hands options directly to the underlying fetch implementation
  • Axios config accepts a new fetcher property which overrides the native fetch implementation, if any

Invasive (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(…)) or axios(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 a URL object
  • axios config now accepts a URL object in the url property
  • axios config now supports a parsedUrl property, for caching a pre-existing URL object
  • axios constructor now accepts a Request object

In cases where axios interfaces accept new objects that then need to be translated to Axios objects (namely, a Request or a Response), there will be a well-defined protocol and set of tests to make sure these changes work as intuitively as possible. For example, copying Headers 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.

@sgammon
Copy link
Author

sgammon commented Oct 18, 2022

@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 http (platform support allowing), and (2) cleaner testsuite use since fetch really is just a re-implementation of the node and browser suites.

@jasonsaayman jasonsaayman self-requested a review October 18, 2022 09:48
@sgammon
Copy link
Author

sgammon commented Nov 14, 2022

@jasonsaayman nearly done with the testsuite here and will rebase/push up soon. could i get a review? wdyt of the approach?

@bumi
Copy link

bumi commented Nov 14, 2022

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

@sgammon
Copy link
Author

sgammon commented Nov 14, 2022

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

@sgammon
Copy link
Author

sgammon commented Nov 14, 2022

rebased and dist updated from v1.x upstream

@jasonsaayman jasonsaayman self-assigned this Nov 15, 2022
@jasonsaayman
Copy link
Member

jasonsaayman commented Nov 15, 2022

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

Copy link

@github-advanced-security github-advanced-security bot left a 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.

@escapedcat
Copy link

Gave it a try by using your fork/branch directly.
It's a simple axios.get.
With the default adapter response.data is filled. Switching to the fetchAdapter response.data is undefined.
The response object in general looks correct.
Did you encounter this before?

@sgammon
Copy link
Author

sgammon commented Nov 15, 2022

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

@bumi
Copy link

bumi commented Nov 16, 2022

we're running it in the browser.

@sgammon
Copy link
Author

sgammon commented Nov 16, 2022

@bumi did you encounter an error as well?

@sgammon
Copy link
Author

sgammon commented Nov 16, 2022

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.

@escapedcat
Copy link

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

bumi was speaking for "us" :) Yes, in the browser.
Didn't see any error, just the empty data.

@sgammon
Copy link
Author

sgammon commented Nov 17, 2022

@escapedcat ah understood! thank you :)

@baraeb92
Copy link

baraeb92 commented Dec 6, 2022

Any ETA when this will land @sgammon ?

@sgammon
Copy link
Author

sgammon commented Dec 7, 2022

@baraeb92 i'm about halfway done with the testsuite refactor, so i would estimate a week or less.

@escapedcat
Copy link

escapedcat commented Dec 7, 2022

Didn't see any error, just the empty data.

This seems to be related to the way I tried to make this work. Imported it like this:

import fetchAdapter from '../node_modules/axios/lib/adapters/fetch.js';

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.

@reslear
Copy link
Contributor

reslear commented Dec 13, 2022

cool, wait :)

@sgammon
Copy link
Author

sgammon commented Dec 14, 2022

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

@bumi
Copy link

bumi commented Dec 14, 2022

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

@sgammon
Copy link
Author

sgammon commented Dec 17, 2022

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.

@sgammon
Copy link
Author

sgammon commented Dec 17, 2022

@jasonsaayman can I get a CI run?

sgammon and others added 10 commits April 11, 2024 14:29
- 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>
@sgammon
Copy link
Author

sgammon commented Apr 12, 2024

Hey everyone,

I've backed up this branch, rebased it against v1.x, and applied necessary fixes/cleanup to get it back to a mergeable state. I will be releasing version 1.6.8-fetch soon so that people can use this new version if they want to. Previous versions will stay online.

I'll look into a v2.x option as well, but no promises yet since I'm not sure how APIs differ.

Testsuites
Tests are passing on:

@sgammon
Copy link
Author

sgammon commented Apr 12, 2024

@jasonsaayman This is now rebased and clean on top of v1.x, but I'm going to return and clean up the whole PR in general. Commits are split right now to keep rebasing easy, but I can definitely drop many of the intermediate commits and make this PR much less invasive in general.

Now that we're rebased on top of v1.x, I'll try to keep it up to date and find a point for good review.

It would be cool to get workflow approval in the meantime so I can make sure CI is green against the main repo.

@PedroS11
Copy link

@jasonsaayman This is now rebased and clean on top of v1.x, but I'm going to return and clean up the whole PR in general. Commits are split right now to keep rebasing easy, but I can definitely drop many of the intermediate commits and make this PR much less invasive in general.

Now that we're rebased on top of v1.x, I'll try to keep it up to date and find a point for good review.

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! ❤️

@johtso
Copy link

johtso commented Apr 15, 2024

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 @mailchimp/mailchimp_transactional in a Cloudflare worker.. but think I'm going to have to wait until timeouts are supported.

@KaKi87
Copy link

KaKi87 commented Apr 29, 2024

WTF ?! Someone just out of the blue dumped 1015 additions and 127 deletions in a single commit implementing a fetch adapter for axios, had it released in beta then commented about it in the issue, while showing no consideration at all about the work being done here. WTF ?!

@sgammon
Copy link
Author

sgammon commented Apr 29, 2024

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 main in one fell swoop, and in smaller chunks.

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 fetch has made it into Axios.

Axios is actually so influential that arguably fetch was heavily influenced by it. It is influential enough and important enough to the wider ecosystem that I'm happy support has landed even if it's not me that got it there.

Thank you everyone for your help getting this across the line, with testing, feedback, and everything else.

@sgammon sgammon closed this Apr 29, 2024
@landsman
Copy link

This is weird...

@DigitalBrainJS
Copy link
Collaborator

@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.
Behind the scenes, research, prototyping, and development of the new Axios core have been ongoing for 2 years as we squeezed almost everything we could out of the current implementation. Without a carefully thought-out system of plugins and hooks, Axios simply gets dirty and grows in size if we try to add some new features, requested by the users. The goal is to make Axios extensible, with a loosely coupled code base, where adapters represent only a minimal layer of logic, and the underlying logic is moved to the core and its plugins to avoid code duplication.
Since technically the fetch adapter had been implemented for a long time locally, but it was expected that it would be released when v2 was ready, the number of issues related to fetch was growing, it was decided to port it partly to the old codebase to release it as a separate PR, at least to "fix" Axios v1. Due to a lack of time and effort, the development and release of V2 were significantly delayed, so I see no reason for users to wait any longer for the fetch adapter.
That PR does not significantly affect the codebase outside of the adapter context and does not introduce breaking changes.
Although a great job has been done in this PR, however, this PR changes the code base too intensively, far beyond the context of the fetch adapter, and adds a cross-fetch dependency, which makes little sense for Node when Axios itself can switch from http to fetch, while at the same time not implementing all the basic xhr functionality yet. All this does not allow us to release it in 1.x.
Unlike rewriting the core, adding an adapter should not significantly affect the rest of the codebase at least at initial PR.
Because this PR undergoes deep refactoring along with the implementation of the adapter, it will be almost impossible to merge all the other PRs in development due to the huge number of conflicts; in fact, you will have to rewrite the code when merging. It's always better to create small PRs that have a single goal and keep potential conflicts to a manageable level.

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

@sgammon
Copy link
Author

sgammon commented Apr 29, 2024

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

@KaKi87
Copy link

KaKi87 commented Apr 29, 2024

Well, not everything that is being worked on is done publicly

It should, that's what openness is.

intensive research work

Which the community could have contributed to.

Behind the scenes, research, prototyping, and development of the new Axios core have been ongoing for 2 years

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.

Although a great job has been done in this PR, however, this PR changes the code base too intensively, far beyond the context of the fetch adapter, and adds a cross-fetch dependency, which makes little sense for Node when Axios itself can switch from http to fetch, while at the same time not implementing all the basic xhr functionality yet. All this does not allow us to release it in 1.x.
Unlike rewriting the core, adding an adapter should not significantly affect the rest of the codebase at least at initial PR.
Because this PR undergoes deep refactoring along with the implementation of the adapter, it will be almost impossible to merge all the other PRs in development due to the huge number of conflicts; in fact, you will have to rewrite the code when merging. It's always better to create small PRs that have a single goal and keep potential conflicts to a manageable level.

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.

@ZhangYu-AX
Copy link

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.

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.

Feature request: fetch based adapter