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 FetchProvider in non-browser environment #7634

Merged
merged 6 commits into from
Oct 23, 2023

Conversation

ocavue
Copy link
Contributor

@ocavue ocavue commented Sep 15, 2023

Discussion

This PR tries get the fetch implementation from not only self but also standard globalThis. This would allow Firebase to get the fetch implementation in non-browser JavaScript environment, like Vercel Edge Runtime.

Closes #7633
Closes #7533

Testing

N/A

API Changes

No API changes

@changeset-bot
Copy link

changeset-bot bot commented Sep 15, 2023

🦋 Changeset detected

Latest commit: 9dda1e5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/auth Patch
@firebase/auth-compat Patch
firebase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@prameshj
Copy link
Contributor

Looks good to me.. @hsubox76 FYI

@hsubox76
Copy link
Contributor

Can you add a changeset? Run yarn changeset. Should be a patch for @firebase/auth. This ensures that this change will be released. The text in the description should be fine for the text in the changeset note.

@ocavue ocavue requested review from a team as code owners September 20, 2023 03:22
@ocavue
Copy link
Contributor Author

ocavue commented Sep 20, 2023

@hsubox76 Changeset file has been added

@EduardJoy
Copy link

Any update on merging this?

@prameshj
Copy link
Contributor

Requesting review from @hsubox76 or @DellaBitta , thanks!

Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to hold this PR up but in the future we should think about putting any changes we need to make global variable checking more robust in this function in firebase/util and using it in all places that need globalThis: https://github.com/firebase/firebase-js-sdk/blob/master/packages/util/src/global.ts#L23 I think it's only used in two places right now but we should check if there's any other instances of self that would benefit from it.

@hsubox76
Copy link
Contributor

Sorry, this is my fault for the delay, but it looks like triggering a re-run of the failed jobs doesn't work because the build artifact it's trying to use has expired. Can you push an empty commit to trigger a new CI run?

@ocavue
Copy link
Contributor Author

ocavue commented Oct 18, 2023

I pushed a new empty commit. The workflow requires approval to run.

@ryohei1216
Copy link

ryohei1216 commented Oct 19, 2023

Sorry, I made the same mistake 🙏
Can you push an empty commit to trigger a new CI run?

@hsubox76 hsubox76 merged commit f002ef3 into firebase:master Oct 23, 2023
26 of 31 checks passed
@google-oss-bot google-oss-bot mentioned this pull request Oct 25, 2023
@firebase firebase locked and limited conversation to collaborators Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants