Skip to content

Mutating global.fetch causes issues with other libraries #113

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

Closed
wilsonzlin opened this issue Jan 17, 2024 · 6 comments
Closed

Mutating global.fetch causes issues with other libraries #113

wilsonzlin opened this issue Jan 17, 2024 · 6 comments

Comments

@wilsonzlin
Copy link

wilsonzlin commented Jan 17, 2024

It appears that this library changes global.fetch if the global fetch does not exist. This causes issues with some other libraries that try to do the same thing, and also overrides the builtin fetch now available in Node.js; in general, mutating globals from a library is not ideal. For example, this caused all fetch calls from the Oracle Cloud SDK to fail, which I have also raised an issue for. As a side note, it also assumes that the platform is Node.js.

Might I suggest using fetch-ponyfill, which provides the same cross-browser fetch polyfill but doesn't mutate the global fetch function? It requires an import, but this should be trivial to migrate to as I found only one call to fetch in this library. Alternatively, perhaps create a local variable like const fetchFn = typeof fetch == "function" ? fetch : require("node-fetch") and use fetchFn instead of mutating global.fetch.

@dsinghvi
Copy link
Contributor

@wilsonzlin thanks for the issue -- Cohere uses Fern to generate their SDKs and we'll be making a change to our generator to fix this!

@hoantran-it
Copy link

I've encountered the same issue with global node-fetch after many hours of debugging. @dsinghvi how can this issue be completely resolved? Thank you!

@billytrend-cohere
Copy link
Collaborator

Hey both, this issues should be resolved in 7.7.4 please let me know if it doesn't fix!

@hoantran-it
Copy link

Hey both, this issues should be resolved in 7.7.4 please let me know if it doesn't fix!

API doesn't work anymore, returns empty when retrieving with for await (const chat of stream)

Screenshot 2024-02-01 at 12 48 36

@billytrend-cohere
Copy link
Collaborator

Hey @hoantran-it super grateful to you for reporting this. We p0'd a fix and 7.7.5 should now be good. We are going to add e2e testing to improve stability of this. Thanks for all of your patience.

@hoantran-it
Copy link

Hey @hoantran-it super grateful to you for reporting this. We p0'd a fix and 7.7.5 should now be good. We are going to add e2e testing to improve stability of this. Thanks for all of your patience.

@billytrend-cohere thank you! It works well now 🥳

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

No branches or pull requests

4 participants