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 for repoGet request #6273

Merged
merged 56 commits into from
Jun 16, 2022
Merged

Fix for repoGet request #6273

merged 56 commits into from
Jun 16, 2022

Conversation

maneesht
Copy link
Contributor

@maneesht maneesht commented May 13, 2022

Fix issue where the wrong cache was being updated by get. First noticed here

@maneesht maneesht requested a review from jsdt as a code owner May 13, 2022 16:09
@changeset-bot
Copy link

changeset-bot bot commented May 13, 2022

🦋 Changeset detected

Latest commit: 7e09d4b

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

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

@maneesht maneesht marked this pull request as draft May 13, 2022 16:10
@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 13, 2022

Size Report 1

Affected Products

  • @firebase/analytics

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser18.9 kB19.0 kB+54 B (+0.3%)
    esm523.5 kB23.6 kB+54 B (+0.2%)
    main24.6 kB24.7 kB+54 B (+0.2%)
    module18.9 kB19.0 kB+54 B (+0.3%)
  • @firebase/auth

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser155 kB155 kB+241 B (+0.2%)
    cordova183 kB183 kB+237 B (+0.1%)
    esm5202 kB203 kB+237 B (+0.1%)
    main148 kB148 kB+237 B (+0.2%)
    module155 kB155 kB+241 B (+0.2%)
    react-native168 kB168 kB+237 B (+0.1%)
  • @firebase/auth-compat

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser20.2 kB20.2 kB+40 B (+0.2%)
    esm527.0 kB27.1 kB+40 B (+0.1%)
    main29.6 kB29.6 kB+40 B (+0.1%)
    module20.2 kB20.2 kB+40 B (+0.2%)
  • @firebase/auth/cordova

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser183 kB183 kB+237 B (+0.1%)
    module183 kB183 kB+237 B (+0.1%)
  • @firebase/auth/internal

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser166 kB166 kB+241 B (+0.1%)
    esm5216 kB216 kB+237 B (+0.1%)
    main183 kB183 kB+237 B (+0.1%)
    module166 kB166 kB+241 B (+0.1%)
  • @firebase/auth/react-native

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser168 kB168 kB+237 B (+0.1%)
    module168 kB168 kB+237 B (+0.1%)
  • @firebase/database

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser248 kB249 kB+1.17 kB (+0.5%)
    esm5276 kB277 kB+1.20 kB (+0.4%)
    main281 kB282 kB+1.20 kB (+0.4%)
    module248 kB249 kB+1.17 kB (+0.5%)
  • @firebase/database-compat/standalone

    TypeBase (497d34c)Merge (04f53fd)Diff
    main370 kB371 kB+1.20 kB (+0.3%)
  • @firebase/firestore

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser257 kB262 kB+4.86 kB (+1.9%)
    esm5319 kB325 kB+5.93 kB (+1.9%)
    main515 kB521 kB+6.06 kB (+1.2%)
    module257 kB262 kB+4.86 kB (+1.9%)
    react-native257 kB262 kB+4.86 kB (+1.9%)
  • @firebase/firestore-lite

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser80.5 kB80.5 kB+2 B (+0.0%)
    esm596.2 kB96.2 kB+2 B (+0.0%)
    main135 kB135 kB+1 B (+0.0%)
    module80.5 kB80.5 kB+2 B (+0.0%)
    react-native80.7 kB80.7 kB+2 B (+0.0%)
  • @firebase/installations

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser17.8 kB17.8 kB+1 B (+0.0%)
    esm522.9 kB22.9 kB+1 B (+0.0%)
    main23.8 kB23.8 kB+1 B (+0.0%)
    module17.8 kB17.8 kB+1 B (+0.0%)
  • @firebase/installations-compat

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser944 B945 B+1 B (+0.1%)
    esm51.16 kB1.16 kB+1 B (+0.1%)
    main1.48 kB1.49 kB+1 B (+0.1%)
    module944 B945 B+1 B (+0.1%)
  • @firebase/messaging

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser21.1 kB21.3 kB+148 B (+0.7%)
    esm526.5 kB26.6 kB+148 B (+0.6%)
    main27.2 kB27.3 kB+148 B (+0.5%)
    module21.1 kB21.3 kB+148 B (+0.7%)
  • @firebase/messaging-sw

    TypeBase (497d34c)Merge (04f53fd)Diff
    main29.9 kB30.0 kB+106 B (+0.4%)
    module23.2 kB23.3 kB+106 B (+0.5%)
  • @firebase/performance

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser29.1 kB29.1 kB+1 B (+0.0%)
    esm530.8 kB30.8 kB+1 B (+0.0%)
    main31.2 kB31.2 kB+1 B (+0.0%)
    module29.1 kB29.1 kB+1 B (+0.0%)
  • @firebase/performance-compat

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser1.10 kB1.10 kB+1 B (+0.1%)
    esm51.37 kB1.38 kB+1 B (+0.1%)
    main1.69 kB1.69 kB+1 B (+0.1%)
    module1.10 kB1.10 kB+1 B (+0.1%)
  • @firebase/remote-config-compat

    TypeBase (497d34c)Merge (04f53fd)Diff
    browser1.85 kB1.85 kB+1 B (+0.1%)
    esm52.76 kB2.76 kB+1 B (+0.0%)
    main3.09 kB3.09 kB+1 B (+0.0%)
    module1.85 kB1.85 kB+1 B (+0.1%)
  • bundle

    31 size changes

    TypeBase (497d34c)Merge (04f53fd)Diff
    analytics (logEvent)41.6 kB41.6 kB+51 B (+0.1%)
    auth (Anonymous)66.1 kB66.3 kB+167 B (+0.3%)
    auth (EmailAndPassword)70.2 kB70.4 kB+167 B (+0.2%)
    auth (GoogleFBTwitterGitHubPopup)90.1 kB90.2 kB+177 B (+0.2%)
    auth (GooglePopup)89.8 kB90.0 kB+177 B (+0.2%)
    auth (GoogleRedirect)90.0 kB90.2 kB+177 B (+0.2%)
    auth (Phone)76.2 kB76.4 kB+167 B (+0.2%)
    database (Append to a list of data)145 kB145 kB-40 B (-0.0%)
    database (Filtering data)144 kB144 kB-40 B (-0.0%)
    database (Listen for child events)160 kB160 kB+218 B (+0.1%)
    database (Listen for value events + Detach listeners)160 kB160 kB+218 B (+0.1%)
    database (Listen for value events)160 kB160 kB+218 B (+0.1%)
    database (Read data once)152 kB157 kB+5.18 kB (+3.4%)
    database (Save data as transactions)162 kB162 kB+218 B (+0.1%)
    database (Sort data)146 kB146 kB-40 B (-0.0%)
    database (Write data)144 kB144 kB-40 B (-0.0%)
    firestore (Persistence)269 kB273 kB+4.75 kB (+1.8%)
    firestore (Query Cursors)208 kB210 kB+2.29 kB (+1.1%)
    firestore (Query)209 kB211 kB+2.29 kB (+1.1%)
    firestore (Read data once)197 kB200 kB+2.28 kB (+1.2%)
    firestore (Realtime updates)200 kB202 kB+2.28 kB (+1.1%)
    firestore (Transaction)181 kB183 kB+2.16 kB (+1.2%)
    firestore (Write data)181 kB183 kB+2.40 kB (+1.3%)
    firestore-lite (Query Cursors)208 kB210 kB+2.29 kB (+1.1%)
    firestore-lite (Query)208 kB210 kB+2.29 kB (+1.1%)
    firestore-lite (Read data once)197 kB200 kB+2.28 kB (+1.2%)
    firestore-lite (Transaction)181 kB183 kB+2.16 kB (+1.2%)
    firestore-lite (Write data)181 kB183 kB+2.40 kB (+1.3%)
    messaging (send + receive)45.0 kB45.1 kB+137 B (+0.3%)
    performance (trace)49.6 kB49.6 kB+2 B (+0.0%)
    remote-config (getAndFetch)44.2 kB44.2 kB+1 B (+0.0%)

  • firebase

    25 size changes

    TypeBase (497d34c)Merge (04f53fd)Diff
    firebase-analytics-compat.js25.6 kB25.6 kB+42 B (+0.2%)
    firebase-analytics.js111 kB111 kB+93 B (+0.1%)
    firebase-app-check.js90.2 kB90.2 kB+20 B (+0.0%)
    firebase-app.js87.6 kB87.6 kB+20 B (+0.0%)
    firebase-auth-compat.js125 kB125 kB+200 B (+0.2%)
    firebase-auth-cordova.js472 kB472 kB+371 B (+0.1%)
    firebase-auth-react-native.js495 kB495 kB+391 B (+0.1%)
    firebase-auth.js417 kB418 kB+395 B (+0.1%)
    firebase-compat.js788 kB794 kB+5.77 kB (+0.7%)
    firebase-database-compat.js166 kB166 kB+588 B (+0.4%)
    firebase-database.js604 kB607 kB+2.62 kB (+0.4%)
    firebase-firestore-compat.js309 kB314 kB+4.76 kB (+1.5%)
    firebase-firestore-lite.js837 kB845 kB+8.10 kB (+1.0%)
    firebase-firestore.js837 kB845 kB+8.10 kB (+1.0%)
    firebase-functions.js32.0 kB32.1 kB+20 B (+0.1%)
    firebase-messaging-compat.js37.9 kB38.1 kB+185 B (+0.5%)
    firebase-messaging-sw.js107 kB107 kB+163 B (+0.2%)
    firebase-messaging.js106 kB106 kB+223 B (+0.2%)
    firebase-performance-compat.js30.7 kB30.7 kB+3 B (+0.0%)
    firebase-performance-standalone-compat.es2017.js87.5 kB87.5 kB+4 B (+0.0%)
    firebase-performance-standalone-compat.js65.3 kB65.3 kB+3 B (+0.0%)
    firebase-performance.js123 kB123 kB+22 B (+0.0%)
    firebase-remote-config-compat.js27.3 kB27.3 kB+2 B (+0.0%)
    firebase-remote-config.js113 kB113 kB+21 B (+0.0%)
    firebase-storage.js146 kB146 kB+20 B (+0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/MHXRqnueKP.html
@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 13, 2022

Size Analysis Report 1

This report is too large (925,925 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

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/L2lqvZNuXN.html
@maneesht maneesht requested a review from jmwski May 20, 2022 15:29
packages/database/src/core/Repo.ts Outdated Show resolved Hide resolved
packages/database/src/core/Repo.ts Outdated Show resolved Hide resolved
packages/database/src/core/Repo.ts Outdated Show resolved Hide resolved
packages/database/test/exp/integration.test.ts Outdated Show resolved Hide resolved
@jmwski jmwski assigned maneesht and unassigned jmwski May 20, 2022
Copy link
Contributor

@jmwski jmwski left a comment

Choose a reason for hiding this comment

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

LGTM after some docs are added. Ignore the note about testing refactor (or take it as a suggestion for a followup PR).

packages/database/src/core/Repo.ts Show resolved Hide resolved
packages/database/src/core/SyncTree.ts Show resolved Hide resolved
packages/database/src/core/SyncTree.ts Show resolved Hide resolved
packages/database/src/core/SyncTree.ts Outdated Show resolved Hide resolved
packages/database/test/exp/integration.test.ts Outdated Show resolved Hide resolved
@jmwski
Copy link
Contributor

jmwski commented May 25, 2022

packages/database/src/core/SyncTree.ts Show resolved Hide resolved
yarn.lock Outdated
@@ -17099,7 +17099,7 @@ uuid@^3.0.0, uuid@^3.3.2, uuid@^3.3.3:
resolved "https://registry.npmjs.org/uuid/-/uuid-3.4.0.tgz#b23e4358afa8a202fe7a100af1f5f883f02007ee"
integrity sha512-HjSDRw6gZE5JMggctHBcjVak08+KEVhSIiDzFnT9S9aegmp85S/bReBVTb4QTFaRNptJ9kuYaNhnbNEOkbKb/A==

uuid@^8.3.2:
uuid@^8.0.0, uuid@^8.3.2:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional? Seems like we shouldn't need to pull in an older version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I assumed yarn would've resolved to the existing one installed in the repo

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is still here?

packages/database/test/exp/integration.test.ts Outdated Show resolved Hide resolved
@jmwski
Copy link
Contributor

jmwski commented Jun 8, 2022

Latest commit w new tests LGTM. @jsdt could you have a look for code-owner approval?

packages/database/test/transport.test.ts Outdated Show resolved Hide resolved
@maneesht maneesht assigned jsdt and unassigned maneesht Jun 15, 2022
@jsdt jsdt assigned maneesht and unassigned jsdt Jun 15, 2022
@@ -56,7 +56,8 @@
"@firebase/app": "0.7.25",
"rollup": "2.72.1",
"rollup-plugin-typescript2": "0.31.2",
"typescript": "4.2.2"
"typescript": "4.2.2",
"uuid": "^8.3.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid bringing in an extra dep here? I see in app-check we just implemented a function ourselves, is this good enough?

export function uuidv4(): string {

If it is, maybe we can extract it into @firebase/util and share it between app-check and database (maybe in a later PR, just have a duplicate implementation in this one?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let me just reimplement it here and add a TODO

@maneesht maneesht merged commit 578dc58 into master Jun 16, 2022
@maneesht maneesht deleted the fix-get-cache branch June 16, 2022 16:05
@google-oss-bot google-oss-bot mentioned this pull request Jun 21, 2022
@firebase firebase locked and limited conversation to collaborators Jul 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
5 participants