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 #241

Open
johansten opened this issue Mar 5, 2019 · 10 comments
Open

Replace Axios with Fetch #241

johansten opened this issue Mar 5, 2019 · 10 comments
Labels
Hacktoberfest suggested issues for Hacktoberfest

Comments

@johansten
Copy link

Axios browserified doesn't support setting 'keep-alive' (the httpsAgent needed is just a stub), so every new server connection will do SSL negotiation, adding quite some time (~0.25s perhaps) to the response time.

isomorphic-fetch comes with node and browser support, with polyfills for older browsers.

I can do the PR, just want a go/no-go

@orbitlens
Copy link

After building with webpack the result package shows that axios static size is 36.2 KB, or 11.6 KB gzipped.

@morleyzhi
Copy link
Contributor

@johansten Go for it!

@vcarl
Copy link
Contributor

vcarl commented Mar 5, 2019

I'd suggest avoiding pulling in any polyfills if possible. sokra sums up my thoughts on it here, it leads to redundant code for library consumers, and fetch is common enough that I think it's reasonable to ask apps to provide it if the environment doesn't.

@morleyzhi
Copy link
Contributor

@vcarl I think we can pull it out in a later, breaking change update, but for now I'd like to preserve compatibility between releases as much as possible.

@vcarl
Copy link
Contributor

vcarl commented Mar 5, 2019

For sure, that makes sense.

@morleyzhi
Copy link
Contributor

morleyzhi commented Mar 5, 2019

I ended up removing an eventsource polyfill and leaving it up to consumers to include, so @johansten I wouldn't include a cross-browser polyfill for it anymore! I used the isNode library to apply that polyfill for node:

https://github.com/stellar/js-stellar-sdk/pull/239/files#diff-41147c007709fcf3e884eb6268314e81R4

@morleyzhi
Copy link
Contributor

Just so you know, I have a PR in that might make it to next week's release that uses Axios infrastructure:

#249

If you're still planning to pursue a fix to this, you'll need to take into account this feature.

@johansten
Copy link
Author

johansten commented Mar 14, 2019

@morleyzhi
I don't it it will cause too much problem. I will most likely have to create some helper functions to make things compatible with axios anyway, as Fetch doesn't have support straight out of the box for the timeouts or content-length checks that are being used in some places.

Edit: Yeah, maybe not. I'll put this on hold. Keep-alive would have been awesome though ¯\_(ツ)_/¯

@tomquisel
Copy link
Contributor

@johansten ah shoot, your idea seemed like a great change! Is adding replacements for those axios features on top of fetch pretty difficult?

@johansten
Copy link
Author

@tomquisel

It doesn't look very complicated.. I'll just replace HorizonAxiosClient with the same functionality implemented using Fetch instead. One issue might be that proper timeouts w/ Fetch needs something called an AbortController which came into full support later than Fetch itself.

@abuiles abuiles added the Hacktoberfest suggested issues for Hacktoberfest label Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest suggested issues for Hacktoberfest
Projects
None yet
Development

No branches or pull requests

7 participants
@abuiles @johansten @tomquisel @vcarl @morleyzhi @orbitlens and others