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

fix: Please consider using fetch instead of XMLHttpRequest #29 #30

Closed
wants to merge 1 commit into from
Closed

fix: Please consider using fetch instead of XMLHttpRequest #29 #30

wants to merge 1 commit into from

Conversation

fishcakeday
Copy link

Replace cross-fetch with @inrupt/universal-fetch to fix #29 and enable usage in Worker edge environment.

@bumi
Copy link
Contributor

bumi commented May 13, 2023

Thanks for the PR and raising this!
@inrupt/universal-fetch does not seem to be much widespread?
I am actually tempted to just drop cross-fetch and leave it up to the user of the library to make sure there is a global fetch available - which is slowly universal, just older node versions have the issue and are still used.

@fishcakeday
Copy link
Author

This will work too, I am uncertain if this package needs support for ancient browsers[1] that do not have fetch() implemented. And node usage can be covered by node-fetch[2]. What do you think?

[1] https://caniuse.com/fetch
[2] https://www.npmjs.com/package/node-fetch

@bumi
Copy link
Contributor

bumi commented May 13, 2023

Browser are fine, but I care about node and the dev experience there.

but I guess we can throw an error if global.fetch is not defined and tell the user to install a global fetch option?

something that we do for WebSockets in the alby-js-sdk: https://github.com/getAlby/alby-js-sdk/blob/master/src/webln/NostrWeblnProvider.ts#L75-L78

do you think this is good?

@fishcakeday
Copy link
Author

That definitely solves the issue. I am uncertain if it is good, but if you are already doing similar thing in the other package, then it wouldn't hurt.

@rolznz rolznz mentioned this pull request May 17, 2023
@rolznz
Copy link
Contributor

rolznz commented Jun 1, 2023

Closing as fixed by #32

@fishcakeday Thanks for reporting this issue and creating the original PR!

@rolznz rolznz closed this Jun 1, 2023
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.

Please consider using fetch instead of XMLHttpRequest
3 participants