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

Replace axios with fetch based alternative #373

Closed
AuHau opened this issue Jul 26, 2021 · 11 comments · Fixed by #390
Closed

Replace axios with fetch based alternative #373

AuHau opened this issue Jul 26, 2021 · 11 comments · Fixed by #390
Labels
kind:architecture Core architecture of project kind:refactor Refactoring issue type:issue

Comments

@AuHau
Copy link
Contributor

AuHau commented Jul 26, 2021

Currently, we are using axios library to make HTTP requests. It is based on an XHR client in a browser that is old and obsolete and is not further developed. fetch is the current standard for making modern requests. It for example supports streaming responses from server, which XHR does not support. And in future it will most probably support even streaming requests. We should therefore switch to either native fetch or some fetch's based library.

I propose to use ky (or its isomorphic version ky-universal) that shares a lot of the same API with fetch but adds some small but important behaviors and functionality like:

  • Simpler API
  • Method shortcuts (ky.post())
  • Treats non-2xx status codes as errors (after redirects)
  • Retries failed requests
  • JSON option
  • Timeout support
  • URL prefix option
  • Instances with custom defaults
  • Hooks
@AuHau AuHau added kind:architecture Core architecture of project kind:refactor Refactoring issue labels Jul 26, 2021
@mattiaz9
Copy link

No upload progress is a big deal breaker for us, we'll have to drop this dependency now.
Why not giving the option to provide a custom fetch implementation? similar to what other packages do, like ccxt.

@AuHau
Copy link
Contributor Author

AuHau commented Aug 30, 2021

@mattiaz9 sorry to hear that, but I am happy you raised your issue!

Well, we could do that, but I am not sure if it would solve anything. Are you developing on browser-side or NodeJS?

To my knowledge, it is not possible to provide nor mock the upload tracking in browsers. If you know about a way please let me know. Well, the only thing I can think of is an XHR wrapper that has a fetch-like API 😅 That sounds like a scary movie thought 🙈

It is possible to do this in NodeJS and it is something we plan to do as per this issue: #397

@mattiaz9
Copy link

@AuHau I'm developing on browser not Node.
I'm not sure how it could be done, or how easy It'd be, since, if I'm not mistaken, you used a custom fetch implementation.

I found this: https://github.com/lifeomic/axios-fetch which should build a fetch like function using axios. So I believe it could be possible, but you'll need to provide an option to override the default implementation.

@AuHau
Copy link
Contributor Author

AuHau commented Aug 30, 2021

Well, I have not exactly used custom fetch per se, but rather a utility that extends bit its functionality (ky). That said I think it allows to use of a custom fetch implementation on its own. See it for example here: https://github.com/sindresorhus/ky-universal/blob/main/index.js#L8

But even if ky would use this custom axios-fetch implementation you would still not get any information about the upload progress right? We would have to expose that somehow...

@AuHau
Copy link
Contributor Author

AuHau commented Aug 30, 2021

Or I guess you could use its "Transforming requests" functionality to catch the request and append an upload-progress callback to it and then track the progress? 🤔

That said, this workaround would for example break the download streaming which was one of the reason for adopting fetch.

@AuHau
Copy link
Contributor Author

AuHau commented Aug 30, 2021

Btw. please also comment on this issue to push the WHATWG to implement proper upload progress tracking for fetch.

@mattiaz9
Copy link

Or I guess you could use its "Transforming requests" functionality to catch the request and append an upload-progress callback to it and then track the progress? 🤔

That said, this workaround would for example break the download streaming which was one of the reason for adopting fetch.

I'm not familiar with the download streaming. That being said the option to override fetch could be implemented in only the api that allows that. Regarding the upload progress, that would be managed by the axios instance so in theory you wouldn't need to expose anything.

Example:

const axiosInstance = axios.create({
  onUploadProgress: () => {...}
})
const fetch = buildAxiosFetch(axiosInstance, ...)
const fileHash = await bee.uploadData(batchId, "Bee is awesome!",  { fetch })

@AuHau
Copy link
Contributor Author

AuHau commented Aug 30, 2021

I see, that is interesting. We will discuss this. Thanks for the input!

@AuHau
Copy link
Contributor Author

AuHau commented Aug 31, 2021

@mattiaz9 we discussed this and we are on-board for this solution. We will validate that the solution you propose indeed will yield the wanted result and if so, then we will implement it! 👍

@mattiaz9
Copy link

@AuHau sounds great, thanks!

@AuHau
Copy link
Contributor Author

AuHau commented Sep 8, 2021

@mattiaz9 just to update. I was able to create a prototype example that made working this approach although the axios-fetch needed bit hacking, which is now put into PR. Feel free to endorse it. So will move further with implementing support for it in bee-js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:architecture Core architecture of project kind:refactor Refactoring issue type:issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants