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

Added timeout fix for axiosInstance #198

Merged
merged 1 commit into from
Sep 24, 2020
Merged

Conversation

AlexZeDim
Copy link
Contributor

I do not remember, have I created this issue or not, but defaultAxios doesn't have timeout at all.

What problem it could generate? If battle.net endpoint somehow doesn't reply you back or packets will be missing, you will never receive the response at all.

For example, if I request data about 10 characters one-by-one, and on the 3rd character request I don't receive a reply from API, well it will block the main thread. Literally forever.

So this little string adds a timeout to axios instance, which will reject the request by timeout. I also understood that according to this issue the axios will be removed by the native fetch method, which doesn't support the timeouts at all. But by creating this pull request I just notify that the problem exists and it deserves any attention.

Okay, to be honest, according to this issue thread in the fetch repo most of the devs think about the timeout option as userland, so in that case, it should be implemented manually.

from default axios timeout to 10s
@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #198 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #198   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           39        39           
  Lines          341       341           
  Branches        40        40           
=========================================
  Hits           341       341           
Impacted Files Coverage Δ
src/helpers/fetch/fetchFromUri.ts 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3883229...326be11. Read the comment docs.

@AlexZeDim AlexZeDim changed the title Update fetchFromUri.ts Added timeout fix for axiosInstance Sep 23, 2020
@lwojcik
Copy link
Member

lwojcik commented Sep 24, 2020

@AlexZeDim thanks for the PR! Yeah, this is something that is definitely missing and it badly needs to be fixed.

In a later iteration I'll make timeout value configurable via an optional property, for now I'm merging as-is. Npm update will arrive later today.

@lwojcik lwojcik merged commit b7427e4 into blizzapi:master Sep 24, 2020
@AlexZeDim AlexZeDim deleted the patch-1 branch September 24, 2020 08:30
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.

None yet

2 participants