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

Get the proper global object depending on environment to check for existing fetch #956

Closed
wants to merge 3 commits into from

Conversation

jcs224
Copy link

@jcs224 jcs224 commented Apr 19, 2021

This might be more of a conversation than a pull request, but I want to attempt to address an issue that's been noticed in a dependency, cross-fetch, that this might help solve.

As it stands, XMLHttpRequest doesn't exist in some runtimes, such as Deno or Cloudflare Workers. Tweaking this library might have a positive cascade of solving some issues, such as these:

supabase/supabase-js#154
lquixada/cross-fetch#78
supabase/supabase-js#161

@JakeChampion
Copy link
Owner

@jcs224, we already have similar code in-place which only adds the polyfills to the global object if the native fetch does not exist:
https:/github/fetch/blob/master/fetch.js#L600-L605

This package wasn't written with those runtimes (deno, cloudflare workers) in mind, is there a way to run our test suite in those runtimes?

@jcs224
Copy link
Author

jcs224 commented Apr 19, 2021

Thanks for the feedback. Now that you mention that snippet, I think the problem is that the global object doesn't exist in environments outside of Node, which is the reason it's breaking outside of Node and normal browser environments. globalThis is supposed to be the way around it, but it's not available in some older environments. I made an update that accounts for the variations in the global namespace, taken directly from MDN:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#search_for_the_global_across_environments

I'm not sure how to test this in the other environments, to be honest. I only just started getting into Deno and just heard about Cloudflare Workers, so more guidance might be needed there. At the very least, don't what to disrupt the library for anyone else using it.

@jcs224 jcs224 changed the title Use existing fetch if XMLHttpRequest not available Get the proper global object depending on environment to check for existing fetch Apr 20, 2021
@jcs224
Copy link
Author

jcs224 commented Apr 20, 2021

@JakeChampion I changed the title of the PR to more accurately reflect the actual change I'm proposing.

@JakeChampion
Copy link
Owner

@jcs224 we went with a different approach then the one you proposed in this pull-request

We now set the variable to an empty object if none of the expected globalThis names work -- #967

@jcs224
Copy link
Author

jcs224 commented May 11, 2021

@JakeChampion excellent, thanks!

@jcs224 jcs224 closed this May 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants