typesense-js icon indicating copy to clipboard operation
typesense-js copied to clipboard

feat: Switch from axios to fetch

Open viktorlarsson opened this issue 1 year ago • 4 comments
trafficstars

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 avatar Aug 23 '24 11:08 viktorlarsson

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

tharropoulos avatar Aug 23 '24 11:08 tharropoulos

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.

viktorlarsson avatar Aug 23 '24 12:08 viktorlarsson

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

tharropoulos avatar Aug 23 '24 12:08 tharropoulos

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?

viktorlarsson avatar Aug 24 '24 12:08 viktorlarsson

any update on this, i spent a day trying to debug cloudflare page only to find out it is typesense

zhihengGet avatar Nov 21 '24 21:11 zhihengGet

Hi. Just wanted to comment. We're using react-native 0.76.3 and in the New Architecture we get

The package at "node_modules/axios/dist/node/axios.cjs" attempted to import the Node standard library module "http".

Would really support this move to fetch as its included in React Native

<3

steflsd avatar Nov 27 '24 05:11 steflsd