-
Notifications
You must be signed in to change notification settings - Fork 48
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
Comments
@JoshuaWR : Can you take a look ? I think we should be able to replace global fetch with the imported fetch. |
I can take a look. Thank you @wilsonzlin for bringing this to our attention, I will look into it and get back to you. |
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? |
I believe the import of isomorphic-fetch itself will mutate the global I also found cross-fetch which is more widely used and supported and may be an alternative to fetch-ponyfill. |
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, allfetch
calls from this SDK failed when I loaded the cohere-ai library, which also mutatesglobal.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 importedfetch
(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
andnode-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 bothwhatwg-fetch
for browsers andnode-fetch
for Node.js. (It appears that node-fetch is also only used in 1 place.)The text was updated successfully, but these errors were encountered: