refactor: axios to fetch

ArweaveTeam / arweave-js

Browser and Nodejs client for general interaction with the arweave protocol and gateways

MIT License

588 stars 129 forks source link

refactor: axios to fetch #192

Closed martonlederer closed 1 year ago

martonlederer commented 1 year ago

This pull request refactors the arweave-js codebase to use the fetch API instead of the axios module. This fixes the incompatibility with service workers and chrome extensions based on the Manifest v3 standard.

Closes #184

martonlederer commented 1 year ago

Status of this PR:

Tests for the web bundle all pass, so it should work for the browser
Nodejs version tests fail, the Headers class used by fetch is undefined and my solution to polyfill it does not work (polyfill code here)

Any help is appreciated!

martonlederer commented 1 year ago

@rosmcmahon if you have a little bit of time to look at this, that would help massively as well

martonlederer commented 1 year ago

One possible solution could be to utilize the same technique that the crypto-driver uses in arweave-js, but I feel like this would be an overkill (?)

rosmcmahon commented 1 year ago

https://github.com/ArweaveTeam/arweave-js/blob/master/src/common/lib/api.ts#L46-L59

  public async get<T = any>(
    endpoint: string,
    config?: AxiosRequestConfig
  ): Promise<AxiosResponse<T>> {
    try {
      return await this.request().get<T>(endpoint, config);
    } catch (error: any) {
      if (error.response && error.response.status) {
        return error.response;
      }

      throw error;
    }
  }

ah. i didn't realise the purpose of the Axios error handling in this function before. it's making it like Fetch API:

just return http status codes without throwing an error.
only throw on connection errors.

so there has been some work done in preparing arweave-js for conversion to fetch already...

martonlederer commented 1 year ago

Yeah exactly, that's what I realized too. Altho this could have been done more easily, with validateStatus.

rosmcmahon commented 1 year ago

i'm looking at raising the minimum node.js version to 16 and using the new built-in Fetch API

martonlederer commented 1 year ago

i'm looking at raising the minimum node.js version to 16 and using the new built-in Fetch API

Would be the easiest solution. Do you think it would cause mayor incompatibilities? LTS is 18.12.1 right now, so i think it would be fine.

rosmcmahon commented 1 year ago

hey @martonlederer can i get push access to your repo?

martonlederer commented 1 year ago

@rosmcmahon you should be invited now

rosmcmahon commented 1 year ago

no Windows laptop handy. troubleshooting through github actions 😃

rosmcmahon commented 1 year ago

all tests passing...

rosmcmahon commented 1 year ago

thoughts about using nodejs Fetch API:

it's marked experimental, I have already seen some typing changes while using it over the last couple of months, we will need to keep an eye on it for changes, but this is not a big deal i think. builds break, you fix them.
if someone wants to use nodejs < 16 they can install a previous version of arweave-js
it's the future!

rosmcmahon commented 1 year ago

i'm not gonna publish this straight away. maybe another set of eyes first, and then update the minor version.

can you be the extra set of eyes @hlolli ?

martonlederer commented 1 year ago

I'll also do some tests in ArConnect. Thanks @rosmcmahon!

rosmcmahon commented 1 year ago

yeah, great work @martonlederer !! thanks for taking the time on this important PR! 😃