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

Add GMPID Header to Firestore #3888

Merged
merged 14 commits into from
Mar 30, 2021
Merged

Add GMPID Header to Firestore #3888

merged 14 commits into from
Mar 30, 2021

Conversation

schmidt-sebastian
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Oct 2, 2020

🦋 Changeset detected

Latest commit: 0a4861e

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

This PR includes changesets to release 8 packages
Name Type
@firebase/firestore Patch
firebase Patch
@firebase/rules-unit-testing 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

@@ -51,6 +55,7 @@ function createMetadata(databasePath: string, token: Token | null): Metadata {
}
}
}
metadata.set('x-firebase-gmpid', appId);
Copy link
Contributor

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2020

Changeset File Check ✅

  • No modified packages are missing from the changeset file.
  • No changeset formatting errors detected.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 2, 2020

Binary Size Report

Affected SDKs

  • @firebase/firestore

    Type Base (b9087b9) Head (92c093a) Diff
    browser 249 kB 249 kB +117 B (+0.0%)
    esm2017 197 kB 197 kB +117 B (+0.1%)
    main 484 kB 484 kB +220 B (+0.0%)
    module 247 kB 247 kB +117 B (+0.0%)
    react-native 197 kB 197 kB +117 B (+0.1%)
  • @firebase/firestore/exp

    Type Base (b9087b9) Head (92c093a) Diff
    browser 189 kB 189 kB +102 B (+0.1%)
    main 477 kB 477 kB +199 B (+0.0%)
    module 189 kB 189 kB +102 B (+0.1%)
    react-native 189 kB 189 kB +102 B (+0.1%)
  • @firebase/firestore/lite

    Type Base (b9087b9) Head (92c093a) Diff
    browser 63.4 kB 63.5 kB +75 B (+0.1%)
    main 140 kB 140 kB +107 B (+0.1%)
    module 63.4 kB 63.5 kB +75 B (+0.1%)
    react-native 63.6 kB 63.7 kB +75 B (+0.1%)
  • @firebase/firestore/memory

    Type Base (b9087b9) Head (92c093a) Diff
    browser 187 kB 187 kB +117 B (+0.1%)
    esm2017 147 kB 147 kB +117 B (+0.1%)
    main 357 kB 357 kB +220 B (+0.1%)
    module 185 kB 185 kB +117 B (+0.1%)
    react-native 147 kB 148 kB +117 B (+0.1%)
  • firebase

    Type Base (b9087b9) Head (92c093a) Diff
    firebase-firestore.js 286 kB 286 kB +117 B (+0.0%)
    firebase-firestore.memory.js 226 kB 226 kB +117 B (+0.1%)
    firebase.js 829 kB 829 kB +117 B (+0.0%)

Test Logs

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 2, 2020

Size Analysis Report

Affected Products

No changes between base commit (d2eda13) and head commit (d4faa7a).

@schmidt-sebastian
Copy link
Contributor Author

This is blocked on https://critique-ng.corp.google.com/cl/335516871 (hence the failing tests)

@Feiyang1
Copy link
Member

Feiyang1 commented Mar 23, 2021

Seems we are ready to merge this PR, since https://critique-ng.corp.google.com/cl/335516871 has been submitted?

@schmidt-sebastian
Copy link
Contributor Author

We have been waiting for the CL to be rolled out, but this has now been done.

@schmidt-sebastian schmidt-sebastian merged commit 4cb0945 into master Mar 30, 2021
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/gmpidfire branch March 30, 2021 17:04
@google-oss-bot google-oss-bot mentioned this pull request Mar 30, 2021
@firebase firebase locked and limited conversation to collaborators Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants