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

feat: Switch from axios to fetch #225

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

viktorlarsson
Copy link

@viktorlarsson viktorlarsson commented Aug 23, 2024

Change Summary

Since Cloudflare workers and Vercel Edge runtimes does not support Axios, the intent here is to switch to native fetch (which has been supported since Node v18.0.0). This should probably be a major bump, but trying not to break contracts as much as possible.

Fixes #161

Investigations needed

  • check crypto, http and https support on edge enviromnets
  • creating a pluggable http client, so we could in theory pass axios, or fetch
  • keep alive wont work in edge, maybe add a typeguard for this?
  • make tests pass
  • multiple runtimes during tests? e.g node edge etc

Changes

  • I needed to write a wrapper on fetch, called FetchWithTimeout to support connectionTimeout. Added a new folder called Shared, not sure that is correct but it is not tied to Typesense in that manner.

PR Checklist

@viktorlarsson viktorlarsson changed the title Feat/move from axios to fetch feat: Switch from axios to fetch Aug 23, 2024
@tharropoulos
Copy link
Contributor

This presents a much larger problem regarding current targets and should be discussed extensively.

Currently, the build targets the last two versions of browsers (that includes IE10, for better or for worse) and this would introduce a breaking change.

Another thing to note is that fetch was marked as stable on Node 21, although it was introduced in Node 18.

If support for IE gets dropped, then removing the dependency altogether would provide a much lighter bundle as well

@viktorlarsson
Copy link
Author

viktorlarsson commented Aug 23, 2024

This presents a much larger problem regarding current targets and should be discussed extensively.

Currently, the build targets the last two versions of browsers (that includes IE10, for better or for worse) and this would introduce a breaking change.

Another thing to note is that fetch was marked as stable on Node 21, although it was introduced in Node 18.

If support for IE gets dropped, then removing the dependency altogether would provide a much lighter bundle as well

Great feedback! It is absolutely a breaking change, I agree. Did not think of the IE support to be honest. I could look into making a polyfill.

I'll continue making the last tests pass and then we can see.

@tharropoulos
Copy link
Contributor

This presents a much larger problem regarding current targets and should be discussed extensively.

Currently, the build targets the last two versions of browsers (that includes IE10, for better or for worse) and this would introduce a breaking change.

Another thing to note is that fetch was marked as stable on Node 21, although it was introduced in Node 18.

If support for IE gets dropped, then removing the dependency altogether would provide a much lighter bundle as well

Great feedback! It is absolutely a breaking change, I agree. Did not think of the IE support to be honest. I could look into making a polyfill.

I'll continue making the last tests pass and then we can see.

Thanks for taking the time to contribute!

Another thing we could look into is the handling of polyfilling Node specific modules (in this case https, http and crypto). Would that break Cloudflare / Edge runtime functionality as well?

Because if so, then migrating over to fetch from axios wouldn't resolve the issue by itself, and we'd have to look into that as well. We currently just mark those as external, so it just throws on runtime if any of those modules is accessed by the client

@viktorlarsson
Copy link
Author

viktorlarsson commented Aug 24, 2024

I've added some things as a checklist @tharropoulos, I do understand that people might need axios due to global interceptors and that will break stuff. Is there a way I could get access to run workflows (specifically tests while working on this?

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.

Axios is not supported on Edge Runtime
2 participants