-
Notifications
You must be signed in to change notification settings - Fork 898
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
Remove referrerPolicy
from Cloudflare Worker fetch
requests
#8393
Conversation
🦋 Changeset detectedLatest commit: b1f3860 The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
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 |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (126,839 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
referrerPolicy
from Cloudflare Worker fetch
requests
.changeset/khaki-numbers-nail.md
Outdated
@@ -0,0 +1,7 @@ | |||
--- | |||
'@firebase/auth': minor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a minor for auth? The API doesn't change. I don't think we bump automatically based on dependencies' bump level. I'm not totally sure if changesets does it but I think it also doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, switched Auth to a patch bump instead of a minor bump.
Changeset File Check
|
@@ -148,14 +148,19 @@ export async function _performApiRequest<T, V>( | |||
headers[HttpHeader.X_FIREBASE_LOCALE] = auth.languageCode; | |||
} | |||
|
|||
const fetchArgs: RequestInit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should consider adding some unit tests on _performApiRequest since we introduced new logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
packages/auth/src/api/index.test.ts
Outdated
if (request !== undefined && request.referrerPolicy !== undefined) { | ||
referrerPolicySet = true; | ||
} | ||
return new Promise<never>((_, reject) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: possible to resolve the promise instead of reject?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also curious why these tests have a rejected promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Updated tests so that these mocks resolve.
The Cloudflare Worker runner runtime doesn't support the `fetch` parameter `referrerPolicy` and throws if one is defined. In attempt to better support Cloudlfare, we will remove this parameter from `fetch` requests when we detect the SDK running in a Cloudflare runner enviornment.
Discussion
The Cloudflare Worker runner runtime doesn't support the
fetch
parameterreferrerPolicy
and throws if one is defined. In attempt to better support Cloudlfare, we will remove this parameter fromfetch
requests when we detect the SDK is running in a Cloudflare runner environment.Fixes #8355.
Testing
CI. Updated unit tests to test if
referrerPolicy
is omitted ifisCloudflareWorker()
returns true.API Changes
N/A.