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.

Sign up for GitHub

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

Jump to bottom

AppCheck integration for Firestore #5634

Merged
merged 25 commits into from
Nov 22, 2021
Merged

AppCheck integration for Firestore #5634

merged 25 commits into from
Nov 22, 2021

Conversation

ehsannas
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Oct 18, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: 9e6f82d

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

This PR includes changesets to release 3 packages
Name Type
@firebase/firestore Minor
firebase Minor
@firebase/firestore-compat 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

@google-cla google-cla bot added the cla: yes label Oct 18, 2021
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 18, 2021 •
edited
Loading

Binary Size Report

Affected SDKs

  • @firebase/app-check

    Type Base (65dce18) Head (7e39b75) Diff
    browser 20.4 kB 22.3 kB +1.95 kB (+9.6%)
    esm5 24.3 kB 26.6 kB +2.36 kB (+9.7%)
    main 25.3 kB 27.8 kB +2.53 kB (+10.0%)
    module 20.4 kB 22.3 kB +1.95 kB (+9.6%)

  • @firebase/app-check-compat

    Type Base (65dce18) Head (7e39b75) Diff
    browser 2.13 kB 2.27 kB +142 B (+6.7%)
    esm5 2.33 kB 2.48 kB +142 B (+6.1%)
    main 2.81 kB 2.94 kB +132 B (+4.7%)
    module 2.13 kB 2.27 kB +142 B (+6.7%)

  • @firebase/auth-compat

    Type Base (65dce18) Head (7e39b75) Diff
    browser 19.7 kB 19.8 kB +38 B (+0.2%)
    esm5 26.6 kB 26.6 kB +38 B (+0.1%)
    main 29.1 kB 29.1 kB +49 B (+0.2%)
    module 19.7 kB 19.8 kB +38 B (+0.2%)

  • @firebase/auth/cordova

    Type Base (65dce18) Head (7e39b75) Diff
    browser 179 kB 179 kB +88 B (+0.0%)
    module 179 kB 179 kB +88 B (+0.0%)

  • @firebase/auth/internal

    Type Base (65dce18) Head (7e39b75) Diff
    browser 163 kB 163 kB +102 B (+0.1%)
    esm5 211 kB 211 kB +102 B (+0.0%)
    main 179 kB 179 kB +88 B (+0.0%)
    module 163 kB 163 kB +102 B (+0.1%)

  • @firebase/auth/react-native

    Type Base (65dce18) Head (7e39b75) Diff
    browser 143 kB 162 kB +19.4 kB (+13.6%)
    module 143 kB 162 kB +19.4 kB (+13.6%)

  • @firebase/component

    Type Base (65dce18) Head (7e39b75) Diff
    browser 8.61 kB 6.54 kB -2.07 kB (-24.1%)
    esm5 ? 8.61 kB ? (?)
    module 8.61 kB 6.54 kB -2.07 kB (-24.1%)

  • @firebase/database

    Type Base (65dce18) Head (7e39b75) Diff
    main 280 kB 280 kB +161 B (+0.1%)

  • @firebase/database-compat/standalone

    Type Base (65dce18) Head (7e39b75) Diff
    main 369 kB 369 kB +165 B (+0.0%)

  • @firebase/firestore

    Type Base (65dce18) Head (7e39b75) Diff
    browser 225 kB 227 kB +2.05 kB (+0.9%)
    esm5 282 kB 284 kB +2.29 kB (+0.8%)
    main 423 kB 427 kB +3.91 kB (+0.9%)
    module 225 kB 227 kB +2.05 kB (+0.9%)
    react-native 226 kB 228 kB +2.05 kB (+0.9%)

  • @firebase/firestore-lite

    Type Base (65dce18) Head (7e39b75) Diff
    browser 71.6 kB 72.4 kB +813 B (+1.1%)
    esm5 84.8 kB 85.7 kB +884 B (+1.0%)
    main 123 kB 125 kB +1.35 kB (+1.1%)
    module 71.6 kB 72.4 kB +813 B (+1.1%)
    react-native 71.8 kB 72.6 kB +821 B (+1.1%)

  • @firebase/functions

    Type Base (65dce18) Head (7e39b75) Diff
    browser 8.85 kB 8.99 kB +145 B (+1.6%)
    esm5 10.9 kB 11.1 kB +161 B (+1.5%)
    main 11.7 kB 11.8 kB +161 B (+1.4%)
    module 8.85 kB 8.99 kB +145 B (+1.6%)

  • @firebase/storage

    Type Base (65dce18) Head (7e39b75) Diff
    browser 52.2 kB 54.5 kB +2.31 kB (+4.4%)
    esm5 58.4 kB 60.5 kB +2.12 kB (+3.6%)
    main 53.6 kB 57.3 kB +3.67 kB (+6.9%)
    module 52.2 kB 54.5 kB +2.31 kB (+4.4%)

  • bundle

    Click to show 34 binary size changes.
    Type Base (65dce18) Head (7e39b75) Diff
    analytics (logEvent) ? 34.7 kB ? (?)
    app-check (CustomProvider) ? 26.9 kB ? (?)
    app-check (ReCaptchaEnterpriseProvider) ? 27.8 kB ? (?)
    app-check (ReCaptchaV3Provider) ? 27.8 kB ? (?)
    auth (Anonymous) ? 56.9 kB ? (?)
    auth (EmailAndPassword) ? 61.0 kB ? (?)
    auth (GoogleFBTwitterGitHubPopup) ? 80.7 kB ? (?)
    auth (GooglePopup) ? 80.4 kB ? (?)
    auth (GoogleRedirect) ? 80.6 kB ? (?)
    auth (Phone) ? 66.9 kB ? (?)
    database (Append to a list of data) ? 137 kB ? (?)
    database (Filtering data) ? 136 kB ? (?)
    database (Listen for child events) ? 152 kB ? (?)
    database (Listen for value events) ? 152 kB ? (?)
    database (Read data once) ? 144 kB ? (?)
    database (Save data as transactions) ? 154 kB ? (?)
    database (Sort data) ? 138 kB ? (?)
    database (Write data) ? 136 kB ? (?)
    firestore (Query) ? 190 kB ? (?)
    firestore (Read data once) ? 195 kB ? (?)
    firestore (Realtime updates) ? 180 kB ? (?)
    firestore-lite (Query) ? 59.4 kB ? (?)
    firestore-lite (Read data once) ? 57.2 kB ? (?)
    functions (call) ? 18.3 kB ? (?)
    messaging (send + receive) ? 37.8 kB ? (?)
    performance (trace) ? 42.3 kB ? (?)
    remote-config (getAndFetch) ? 36.7 kB ? (?)
    storage (getDownloadURL) ? 28.7 kB ? (?)
    storage (getMetadata) ? 28.1 kB ? (?)
    storage (list + listAll) ? 27.5 kB ? (?)
    storage (updateMetadata) ? 28.4 kB ? (?)
    storage (uploadBytes) ? 32.9 kB ? (?)
    storage (uploadBytesResumable) ? 42.4 kB ? (?)
    storage (uploadString) ? 33.1 kB ? (?)

  • firebase

    Click to show 28 binary size changes.
    Type Base (65dce18) Head (7e39b75) Diff
    firebase-analytics.js 107 kB 107 kB -193 B (-0.2%)
    firebase-app-check-compat.js 19.9 kB 20.8 kB +894 B (+4.5%)
    firebase-app-check.js 77.6 kB 80.4 kB +2.76 kB (+3.6%)
    firebase-app-compat.js 21.3 kB 17.9 kB -3.38 kB (-15.9%)
    firebase-app.js 59.2 kB 51.3 kB -7.87 kB (-13.3%)
    firebase-auth-compat.js 122 kB 123 kB +845 B (+0.7%)
    firebase-auth-cordova.js 460 kB 91 B -460 kB (-100.0%)
    firebase-auth-react-native.js 430 kB 101 B -430 kB (-100.0%)
    firebase-auth.js 410 kB 410 kB +188 B (+0.0%)
    firebase-compat.js 749 kB 751 kB +1.97 kB (+0.3%)
    firebase-database-compat.js 168 kB 165 kB -2.95 kB (-1.8%)
    firebase-database.js 603 kB 603 kB -193 B (-0.0%)
    firebase-firestore-compat.js 277 kB 280 kB +2.42 kB (+0.9%)
    firebase-firestore-lite.js 244 kB 245 kB +684 B (+0.3%)
    firebase-firestore.js 763 kB 766 kB +2.67 kB (+0.4%)
    firebase-functions-compat.js 7.94 kB 7.95 kB +5 B (+0.1%)
    firebase-functions.js 30.6 kB 30.9 kB +311 B (+1.0%)
    firebase-messaging-compat.js 37.9 kB 38.0 kB +13 B (+0.0%)
    firebase-messaging-sw.js 102 kB 102 kB -193 B (-0.2%)
    firebase-messaging.js 101 kB 101 kB -193 B (-0.2%)
    firebase-performance-compat.js 30.8 kB 30.8 kB -20 B (-0.1%)
    firebase-performance-standalone-compat.es2017.js 82.5 kB 78.9 kB -3.62 kB (-4.4%)
    firebase-performance-standalone-compat.js 54.0 kB 57.0 kB +3.02 kB (+5.6%)
    firebase-performance.js 119 kB 119 kB -193 B (-0.2%)
    firebase-remote-config-compat.js 27.5 kB 27.5 kB -40 B (-0.1%)
    firebase-remote-config.js 109 kB 108 kB -193 B (-0.2%)
    firebase-storage-compat.js 38.2 kB 38.2 kB +60 B (+0.2%)
    firebase-storage.js 141 kB 145 kB +4.44 kB (+3.2%)

Test Logs

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 18, 2021 •
edited
Loading

Size Analysis Report

Affected Products

Diffs between base commit (65dce18) and head commit (7e39b75) are too large (376,989 characters) to display.

Please check below links to see details from the original test log.

schmidt-sebastian
schmidt-sebastian reviewed Oct 18, 2021
View reviewed changes
.changeset/mean-elephants-rush.md Outdated Show resolved Hide resolved
packages/firestore/src/api/credentials.ts Show resolved Hide resolved
packages/firestore/src/api/credentials.ts Outdated Show resolved Hide resolved
packages/firestore/src/api/credentials.ts Outdated Show resolved Hide resolved
packages/firestore/src/core/firestore_client.ts Outdated Show resolved Hide resolved
packages/firestore/src/remote/datastore.ts Outdated Show resolved Hide resolved
packages/firestore/src/remote/datastore.ts Outdated Show resolved Hide resolved
packages/firestore/src/remote/datastore.ts Outdated Show resolved Hide resolved
packages/firestore/src/remote/datastore.ts Outdated Show resolved Hide resolved
packages/firestore/src/remote/persistent_stream.ts Outdated Show resolved Hide resolved
@schmidt-sebastian
Copy link
Contributor

Looks very good! Please take a look at the test failure.

schmidt-sebastian
schmidt-sebastian reviewed Oct 20, 2021
View reviewed changes
packages/firestore/lite/register.ts Outdated Show resolved Hide resolved
packages/firestore/src/api/credentials.ts Outdated Show resolved Hide resolved
packages/firestore/src/api/credentials.ts Outdated Show resolved Hide resolved
packages/firestore/src/api/credentials.ts Outdated Show resolved Hide resolved
packages/firestore/src/api/credentials.ts Outdated Show resolved Hide resolved
packages/firestore/src/api/credentials.ts Outdated Show resolved Hide resolved
packages/firestore/test/unit/remote/rest_connection.test.ts Outdated Show resolved Hide resolved
@ehsannas
Copy link
Contributor Author

Not sure why formatting bot is failing with some formatting errors from Storage. This PR doesn't touch any files from Storage 🤷‍♂️

@schmidt-sebastian
Copy link
Contributor

Not sure why formatting bot is failing with some formatting errors from Storage. This PR doesn't touch any files from Storage 🤷‍♂️

it's brand new so it may need some adjusting. You can reformat storage if this is blocking (or we can create a new PR for this).

@ehsannas
Copy link
Contributor Author

ehsannas commented Oct 20, 2021 •
edited
Loading

it's brand new so it may need some adjusting. You can reformat storage if this is blocking (or we can create a new PR for this).

running yarn prettier does not change anything in Storage in my tree. I guess the bot runs the check on "master + this PR"?

I could rebase, but then I'll have to push --force. In the past GitHub wasn't great with keeping the code review history after a force push. Not sure if it has improved. Will find out shortly!

@egilmorez
Copy link
Contributor

Adding Kevin and Mark for doc review. Thanks!

@ehsannas @schmidt-sebastian
Update .changeset/mean-elephants-rush.md
9e6f82d 
Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
@ehsannas ehsannas merged commit 086df7c into master Nov 22, 2021
@ehsannas ehsannas deleted the ehsann/firestore-appcheck branch November 22, 2021 19:45
@google-oss-bot google-oss-bot mentioned this pull request Dec 1, 2021
Merged
@firebase firebase locked and limited conversation to collaborators Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kevinthecheung kevinthecheung kevinthecheung approved these changes

@schmidt-sebastian schmidt-sebastian schmidt-sebastian approved these changes

@egilmorez egilmorez Awaiting requested review from egilmorez

@markarndt markarndt Awaiting requested review from markarndt

Assignees

@schmidt-sebastian schmidt-sebastian

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ehsannas @google-oss-bot @schmidt-sebastian @egilmorez @kevinthecheung