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

Use of isomorphic-fetch mutates global fetch unnecessarily, causing problems with other libraries #259

Open
wilsonzlin opened this issue Jan 17, 2024 · 4 comments
Labels
SDK Issue pertains to the SDK itself and not specific to any service

Comments

@wilsonzlin
Copy link

wilsonzlin commented Jan 17, 2024

The isomorphic-fetch dependency mutates the global fetch value. This causes problems with some other libraries that also do this in some other way. For example, all fetch calls from this SDK failed when I loaded the cohere-ai library, which also mutates global.fetch (I have raised an issue there too).

It appears that this isn't necessary, as the fetch call is only used in 2 places (excluding examples) across the entire codebase: circuit-breaker.ts and http.ts. These can be trivially migrated to using an imported fetch (see below) that avoids mutating the global value, which is actually already done in url-based-x509-certificate-supplier.ts.

Also, the dependence on both isomorphic-fetch and node-fetch appears to be redundant (the former loads the latter on Node.js). I would suggest replacing both with fetch-ponyfill instead, which is recommended by the authors of isomorphic-fetch; it doesn't mutate the builtin, and provides both whatwg-fetch for browsers and node-fetch for Node.js. (It appears that node-fetch is also only used in 1 place.)

@jyotisaini jyotisaini added the SDK Issue pertains to the SDK itself and not specific to any service label Jan 17, 2024
@jyotisaini
Copy link
Contributor

@JoshuaWR : Can you take a look ? I think we should be able to replace global fetch with the imported fetch.

@JoshuaWR
Copy link
Member

I can take a look. Thank you @wilsonzlin for bringing this to our attention, I will look into it and get back to you.

@JoshuaWR
Copy link
Member

Hi @wilsonzlin, just want to clarify on something. We might not want to migrate to fetch-ponyfill, and instead continue to use isomorphic-fetch. If we include 'import 'isomorphic-fetch' in each file where we call fetch, will this solve the issue you are facing? Or will the inclusion of isomorphic-fetch at all lead to mutation of global fetch?

@wilsonzlin
Copy link
Author

wilsonzlin commented Jan 24, 2024

I believe the import of isomorphic-fetch itself will mutate the global fetch regardless of where or how many times it's imported, and therefore continue this problem according to the docs.

I also found cross-fetch which is more widely used and supported and may be an alternative to fetch-ponyfill.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SDK Issue pertains to the SDK itself and not specific to any service
Projects
None yet
Development

No branches or pull requests

3 participants