Skip to content

Replace axios with native fetch #31

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

Merged
merged 2 commits into from
Apr 5, 2023
Merged

Replace axios with native fetch #31

merged 2 commits into from
Apr 5, 2023

Conversation

mattt
Copy link
Contributor

@mattt mattt commented Mar 30, 2023

Related to #29 (comment)

The decision to use axios was entirely pragmatic — it was the first thing I got working, so I went with that. I'd be very open to dropping it entirely for fetch if that works cross-platform reasonably well.

This PR should do exactly that. I'm out of the loop about preferred fetch polyfill libraries and other packaging concerns, so if you have any strong opinions, please let me know!

@mattt
Copy link
Contributor Author

mattt commented Mar 30, 2023

All of our tests currently stub out request, so there's currently no way to tell if this actually works. We should update our test suite to stub or mock fetch responses or support smoke testing instead.

Converting this PR to draft until we have some coverage to verify these changes.

@mattt mattt marked this pull request as draft March 30, 2023 12:22
@nfcampos
Copy link

Wonderful, this is much better.

What versions of node are you targeting?

the ideal way to use fetch is to 1. accept it as an argument (for unit tests) but default to looking for a global fetch function, which is now available in node 18+ and all other JS envs

@zeke
Copy link
Member

zeke commented Mar 31, 2023

What versions of node are you targeting?

We don't have a documented policy, but LTS (currently >=18)) seems sensible.

@nfcampos
Copy link

Great, then there is no reason to use cross-fetch, as fetch is available globally in node 18+ without a flag

@danielrhodes
Copy link

danielrhodes commented Mar 31, 2023

This is great. Was going to fork this library to use inside a Cloudflare Worker. They don't support node (so axios does not work), but they do support a browser-like fetch. I believe a similar issue would pop up if using this library inside an AWS Lambda Edge function or Vercel Edge functions.

@mattt
Copy link
Contributor Author

mattt commented Apr 5, 2023

Now that we're mocking HTTP responses with nock (#38), this is now ready for review.

@mattt mattt marked this pull request as ready for review April 5, 2023 21:14
@mattt mattt changed the title Replace axios with cross-fetch Replace axios with native fetch Apr 5, 2023
@mattt mattt merged commit b339725 into main Apr 5, 2023
@mattt mattt deleted the mattt/fetch branch April 5, 2023 21:33
@zeke
Copy link
Member

zeke commented Apr 10, 2023

🎉 cc @gr2m

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.

4 participants