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

Implement configureFieldIndexes with tests #6403

Merged
merged 17 commits into from
Jul 26, 2022
Merged

Conversation

tom-andersen
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Jun 30, 2022

🦋 Changeset detected

Latest commit: 82f0e60

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 Patch
firebase Patch
@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-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 30, 2022

Size Report 1

Affected Products

  • @firebase/analytics

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    browser20.1 kB20.1 kB-1 B (-0.0%)
    esm524.7 kB24.7 kB-1 B (-0.0%)
    main26.0 kB26.0 kB-1 B (-0.0%)
    module20.1 kB20.1 kB-1 B (-0.0%)
  • @firebase/app-check

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    browser25.1 kB25.2 kB+92 B (+0.4%)
    esm529.8 kB29.9 kB+92 B (+0.3%)
    main31.0 kB31.1 kB+92 B (+0.3%)
    module25.1 kB25.2 kB+92 B (+0.4%)
  • @firebase/auth

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    browser155 kB155 kB+102 B (+0.1%)
    esm5203 kB203 kB+102 B (+0.1%)
    module155 kB155 kB+102 B (+0.1%)
    react-native168 kB168 kB+102 B (+0.1%)
  • @firebase/auth/internal

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    browser166 kB166 kB+102 B (+0.1%)
    esm5216 kB216 kB+102 B (+0.0%)
    module166 kB166 kB+102 B (+0.1%)
  • @firebase/auth/react-native

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    browser168 kB168 kB+102 B (+0.1%)
    module168 kB168 kB+102 B (+0.1%)
  • @firebase/database

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    browser248 kB248 kB-6 B (-0.0%)
    esm5276 kB276 kB-12 B (-0.0%)
    main282 kB282 kB-12 B (-0.0%)
    module248 kB248 kB-6 B (-0.0%)
  • @firebase/database-compat/standalone

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    main371 kB371 kB-12 B (-0.0%)
  • @firebase/firestore

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    browser262 kB264 kB+1.43 kB (+0.5%)
    esm5325 kB327 kB+1.66 kB (+0.5%)
    main522 kB525 kB+2.92 kB (+0.6%)
    module262 kB264 kB+1.43 kB (+0.5%)
    react-native262 kB264 kB+1.43 kB (+0.5%)
  • bundle

    16 size changes

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    analytics (logEvent)41.8 kB41.8 kB-1 B (-0.0%)
    app-check (ReCaptchaEnterpriseProvider)37.6 kB37.6 kB+42 B (+0.1%)
    app-check (ReCaptchaV3Provider)37.5 kB37.6 kB+42 B (+0.1%)
    auth (Phone)76.4 kB76.5 kB+94 B (+0.1%)
    firestore (Persistence)273 kB274 kB+408 B (+0.1%)
    firestore (Query Cursors)210 kB211 kB+397 B (+0.2%)
    firestore (Query)211 kB212 kB+397 B (+0.2%)
    firestore (Read data once)200 kB200 kB+397 B (+0.2%)
    firestore (Realtime updates)202 kB202 kB+397 B (+0.2%)
    firestore (Transaction)184 kB184 kB+408 B (+0.2%)
    firestore (Write data)183 kB184 kB+408 B (+0.2%)
    firestore-lite (Query Cursors)210 kB67.9 kB-142 kB (-67.7%)
    firestore-lite (Query)210 kB71.1 kB-139 kB (-66.2%)
    firestore-lite (Read data once)200 kB55.5 kB-144 kB (-72.2%)
    firestore-lite (Transaction)184 kB80.1 kB-103 kB (-56.4%)
    firestore-lite (Write data)183 kB65.3 kB-118 kB (-64.4%)

  • firebase

    18 size changes

    TypeBase (1d3a34d)Merge (0b431e7)Diff
    firebase-analytics-compat.js25.8 kB25.8 kB-2 B (-0.0%)
    firebase-analytics.js115 kB115 kB-87 B (-0.1%)
    firebase-app-check-compat.js22.8 kB22.9 kB+70 B (+0.3%)
    firebase-app-check.js90.9 kB90.9 kB-15 B (-0.0%)
    firebase-app.js87.6 kB87.4 kB-172 B (-0.2%)
    firebase-auth-compat.js125 kB125 kB+82 B (+0.1%)
    firebase-auth-react-native.js498 kB498 kB+212 B (+0.0%)
    firebase-auth.js418 kB418 kB+169 B (+0.0%)
    firebase-compat.js794 kB794 kB+555 B (+0.1%)
    firebase-database.js606 kB606 kB-137 B (-0.0%)
    firebase-firestore-compat.js314 kB314 kB+405 B (+0.1%)
    firebase-firestore-lite.js845 kB267 kB-578 kB (-68.4%)
    firebase-firestore.js845 kB851 kB+6.29 kB (+0.7%)
    firebase-functions.js32.1 kB32.0 kB-43 B (-0.1%)
    firebase-installations-compat.js?13.2 kB? (?)
    firebase-installations.js?59.6 kB? (?)
    firebase-performance.js123 kB123 kB-86 B (-0.1%)
    firebase-remote-config.js113 kB113 kB-43 B (-0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/LqxhnZRCpF.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 30, 2022

Size Analysis Report 1

This report is too large (640,509 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/k1cayChwJa.html

@tom-andersen tom-andersen marked this pull request as ready for review June 30, 2022 20:22
@tom-andersen tom-andersen requested a review from wu-hui June 30, 2022 20:22
@@ -473,24 +473,25 @@ export class FirebaseAppCheckTokenProvider
asyncQueue: AsyncQueue,
changeListener: CredentialChangeListener<string>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is just whitespace formatting that happens when the lint:fix and yarn prettier are run.

Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Mostly ready to merge. I have a kind of big ask to add a spec test..

}
const tokenUpdated = tokenResult.token !== this.latestAppCheckToken;
this.latestAppCheckToken = tokenResult.token;
const onTokenChanged: (
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 a merge error?

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 is the linter making things pretty.

It's a minor nuisance to omit committing this file every time I work on JS. So I let it slide this time, and thought it could be included.

This is purely a whitespace change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look like a whitespace change to me..the error message changed for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a closer look. Githubs diff layout is misleading you.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, see it now!

packages/firestore/src/api/index_configuration.ts Outdated Show resolved Hide resolved
packages/firestore/src/api/index_configuration.ts Outdated Show resolved Hide resolved
packages/firestore/src/local/indexeddb_schema.ts Outdated Show resolved Hide resolved
packages/firestore/src/local/local_store_impl.ts Outdated Show resolved Hide resolved
@wu-hui wu-hui assigned tom-andersen and unassigned wu-hui Jul 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 15, 2022

Changeset File Check ✅

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

@tom-andersen tom-andersen assigned wu-hui and unassigned tom-andersen Jul 15, 2022
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Almost LGTM, I believe the app check change worth taking another look.

}
const tokenUpdated = tokenResult.token !== this.latestAppCheckToken;
this.latestAppCheckToken = tokenResult.token;
const onTokenChanged: (
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look like a whitespace change to me..the error message changed for example.

@wu-hui wu-hui assigned tom-andersen and unassigned wu-hui Jul 21, 2022
@tom-andersen tom-andersen assigned wu-hui and unassigned tom-andersen Jul 22, 2022
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Thanks, approved!

}
const tokenUpdated = tokenResult.token !== this.latestAppCheckToken;
this.latestAppCheckToken = tokenResult.token;
const onTokenChanged: (
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, see it now!

@wu-hui wu-hui assigned tom-andersen and unassigned wu-hui Jul 22, 2022
packages/firestore/package.json Outdated Show resolved Hide resolved
packages/firestore/package.json Outdated Show resolved Hide resolved
@tom-andersen tom-andersen merged commit f5426a5 into master Jul 26, 2022
@tom-andersen tom-andersen deleted the tomandersen/indexTests branch July 26, 2022 22:21
@google-oss-bot google-oss-bot mentioned this pull request Aug 4, 2022
@firebase firebase locked and limited conversation to collaborators Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants