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 dependency with native fetch #165

Closed
lwojcik opened this issue Jul 17, 2020 · 3 comments
Closed

Replace axios dependency with native fetch #165

lwojcik opened this issue Jul 17, 2020 · 3 comments

Comments

@lwojcik
Copy link
Member

lwojcik commented Jul 17, 2020

Replace axios dependency with native fetch

https://blog.logrocket.com/axios-or-fetch-api/

@AlexZeDim
Copy link
Contributor

AlexZeDim commented Jul 21, 2020

Is replacing an axios in BlizzAPI so simple, that even I could edit this strings in fetchFromUri.ts

    if (method === 'POST') Object.assign(requestOptions, { params: options.params });
    /* tslint:enable no-object-mutation */
    const response = await fetch.get(uri, requestOptions);
    return response.data;

And it's (almost) all done?

Except this 3 points:

  1. To send data, fetch() uses the body property, while Axios uses the data property
  2. The data in fetch() is stringified
  3. The URL is passed as an argument to fetch(). In Axios, however, the URL is set in the options object

I mean what about error handling?

According to the article above, which, I have read it too:

There is also the fact that axios handles error responses differently from fetch.
For fetch only network errors are actual errors.
For axios perfectly successful server communication that happens to return 400+ responses is also an error.

I am a bit depressed how old blizzard.js and battlenet-api-wrapper handles it. So I rewrite error handling each time in my own fork. So what's the plan for errors? Would every error throw new Error or there will be something else?
(hopes in won't spam in console.error)

@lwojcik
Copy link
Member Author

lwojcik commented Jul 22, 2020

Thanks for the comment. Yes, replacing axios call in fetchFromUri.ts should suffice.

About error handling, you raised a valid point. I haven't thought much about it, as this issue is fairly low on my priority list for now. I'll look at different approaches and comment on this once I form my opinion, hopefully within next few weeks.

@lwojcik
Copy link
Member Author

lwojcik commented Oct 8, 2020

TL;DR: I decided I’m not going to pursue this change in the near future. There’s a lot of work to be done to get it right and I’m not sure if benefits are worth it.

I took some time to analyze this issue and here are my thoughts.

First, some statistics. As of October 2020, out of 15 public projects reported by GitHub as dependent on BlizzAPI, 9 of them are Node.js apps, 5 of them are browser apps and 1 is an environment-agnostic library.

Excluding my own projects, numbers look as follows: 3 Node.js apps and 5 browser apps.

While BlizzAPI was never meant to be a browser library, I’m happy to recognize wide adoption of it among web applications.

Browser-based apps would definitely benefit from using native API rather than third-party implementation. Even though I’d still have to bring a polyfill library (most likely cross-fetch) to polyfill fetch in Node.js, browser apps would use native Fetch anyway. Cross-fetch has relatively straightforward structure and less moving parts than axios.

For Node.js apps replacing one dependency with another isn’t really an issue as long as it APIs that rely on it remain compatible.

However, replacing axios with fetch brings some potential issues:

  • as @AlexZeDim pointed out in the comment above, axios treats 4xx responses as errors and this de facto became a standard behavior for dependent projects to rely on. Implementing compatible behavior with fetch will require adding more boilerplate code to handle it correctly. In other words, more work needed to re-implement and test what axios brings as a highly flexible feature.

  • the point above also applies to fetch timeout, which turned out to be easy to support thanks to axios offering it as a feature. With fetch we’d have had to reimplement it with even more boilerplate code.

  • Even though in BlizzAPI it’s fairly straightforward to replace one data fetching library with another, I’m a bit concerned about other unforeseen issues that could slip in unnoticed by unit tests. me and several other people already rely on BlizzAPI in numerous projects. I myself intend to build more new ones on top of it. My time for open source development is limited and I want to keep it as productive as possible. At this point I’m more willing to keep the project as stable as possible, fix submitted bugs and bring new features rather than implement changes which may deteriorate user experience while bringing no added value.

  • axios brings some interesting features I haven’t had a chance to look at yet but might prove useful in the future, such as interceptors or cancelling requests. Fetch brings no new functionality to the table.

  • Finally, and perhaps the most important, Node.js still requires some kind of polyfill to bring compatibility with window.fetch, with node-fetch being the most popular. As of October 2020 I don’t expect Node.js bringing native fetch too soon.

Based on the above, I’m not going to pursue this change in the near future. There’s much more work to be done to get it right and I’m not sure if this is what I want to do.

I might reconsider my stance on this if any of the following happens:

  • Node.js implements native fetch,
  • anything of importance happens to axios library that causes BlizzAPI to stop working,
  • I observe burning issues specifically related to axios being used in web browsers or Node.js that can’t be fixed in any other way than replacing it completely - in this case feel free to open a new issue.

@lwojcik lwojcik closed this as completed Oct 8, 2020
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

No branches or pull requests

2 participants