-
Notifications
You must be signed in to change notification settings - Fork 927
Add GMPID Header to Firestore #3888
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
Conversation
🦋 Changeset detectedLatest commit: 0a4861e The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
…-sdk into mrschmidt/gmpidfire
@@ -51,6 +55,7 @@ function createMetadata(databasePath: string, token: Token | null): Metadata { | |||
} | |||
} | |||
} | |||
metadata.set('x-firebase-gmpid', appId); |
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.
Question: the plumbing accounts for the possibility of appId
being empty. What do you think about not setting the header at all if appId
is empty rather than setting an "empty" header?
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.
I looked at the RTDB code and it uses an empty app ID and sends the header. I wanted to match that behavior.
@@ -118,6 +118,7 @@ export abstract class RestConnection implements Connection { | |||
token: Token | null | |||
): void { | |||
headers['X-Goog-Api-Client'] = X_GOOG_API_CLIENT_VALUE; | |||
headers['X-Firebase-GMPID'] = this.databaseInfo.appId; |
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.
Nit: capitalization of the header name is different between this file and grpc_connection
. Is this intentional?
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.
The goal was to match the surrounding code. Let me fix the GRPC headers.
Changeset File Check ✅
|
Binary Size ReportAffected SDKs
Test Logs |
Size Analysis Report |
…-sdk into mrschmidt/gmpidfire
This is blocked on https://critique-ng.corp.google.com/cl/335516871 (hence the failing tests) |
Seems we are ready to merge this PR, since https://critique-ng.corp.google.com/cl/335516871 has been submitted? |
We have been waiting for the CL to be rolled out, but this has now been done. |
No description provided.