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

snapshot listeners source from cache #7982

Merged
merged 36 commits into from
Mar 11, 2024
Merged

snapshot listeners source from cache #7982

merged 36 commits into from
Mar 11, 2024

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented Jan 25, 2024 •
edited
Loading

Adding ListenSource enum to SnapshotListenOptions.

const enum ListenSource {
  /** Listen to both cache and server changes */
  Default,
  /** Listen to changes in cache only */
  Cache
}

tests include integration tests and spec tests on:

  • listening/un-listening-relistening to query sourced from cache
  • having multiple listeners from different source
  • raising snapshot from cache, on local mutation, watch updates(while listening to both default and cache) and bundle loads
  • mirror queries in integration test and multi-client spec tests
  • transactions, network changes should not raise snapshot while listening to cache only
  • can execute composite index queries.
  • eager and persistence garbage collection

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 25, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: a340dd6

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

google-oss-bot commented Jan 25, 2024 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/auth

    TypeBase (e60188d)Merge (10acec4)Diff
    browser177 kB177 kB+38 B (+0.0%)
    esm5231 kB231 kB+38 B (+0.0%)
    module177 kB177 kB+38 B (+0.0%)

  • @firebase/auth/internal

    TypeBase (e60188d)Merge (10acec4)Diff
    browser188 kB188 kB+38 B (+0.0%)
    esm5244 kB244 kB+38 B (+0.0%)
    module188 kB188 kB+38 B (+0.0%)

  • @firebase/firestore

    TypeBase (e60188d)Merge (10acec4)Diff
    browser375 kB376 kB+1.29 kB (+0.3%)
    esm5360 kB362 kB+1.60 kB (+0.4%)
    main577 kB579 kB+2.40 kB (+0.4%)
    module375 kB376 kB+1.29 kB (+0.3%)
    react-native375 kB376 kB+1.29 kB (+0.3%)

  • bundle

    11 size changes

    TypeBase (e60188d)Merge (10acec4)Diff
    auth (GoogleFBTwitterGitHubPopup)101 kB101 kB+19 B (+0.0%)
    firestore (CSI Auto Indexing Disable and Delete)268 kB268 kB+57 B (+0.0%)
    firestore (CSI Auto Indexing Enable)268 kB268 kB+57 B (+0.0%)
    firestore (Persistence)303 kB303 kB+101 B (+0.0%)
    firestore (Query Cursors)246 kB247 kB+903 B (+0.4%)
    firestore (Query)243 kB244 kB+936 B (+0.4%)
    firestore (Read data once)231 kB232 kB+903 B (+0.4%)
    firestore (Read Write w Persistence)321 kB322 kB+947 B (+0.3%)
    firestore (Realtime updates)234 kB235 kB+936 B (+0.4%)
    firestore (Transaction)211 kB211 kB+57 B (+0.0%)
    firestore (Write data)211 kB211 kB+57 B (+0.0%)

  • firebase

    TypeBase (e60188d)Merge (10acec4)Diff
    firebase-auth.js147 kB147 kB+19 B (+0.0%)
    firebase-compat.js780 kB781 kB+934 B (+0.1%)
    firebase-firestore-compat.js341 kB342 kB+934 B (+0.3%)
    firebase-firestore.js434 kB436 kB+1.30 kB (+0.3%)

Test Logs

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 25, 2024 •
edited
Loading

Size Analysis Report 1

This report is too large (203,112 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/8WtvmEo4xb.html

@milaGGL milaGGL marked this pull request as ready for review January 29, 2024 22:49
@milaGGL milaGGL self-assigned this Jan 29, 2024
@wu-hui wu-hui self-assigned this Jan 30, 2024
@milaGGL milaGGL requested a review from wu-hui January 30, 2024 20:37
@milaGGL milaGGL changed the title Mila/snapshot listener source from cache snapshot listeners source from cache Jan 30, 2024
@milaGGL milaGGL removed their assignment Jan 30, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 2, 2024 •
edited
Loading

Changeset File Check ✅

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

wu-hui
wu-hui reviewed Feb 9, 2024
View reviewed changes
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.

Another round. I am half way through tests, but I think we should try to make our tests cover different query/collection combinations.

packages/firestore/src/core/sync_engine_impl.ts Outdated Show resolved Hide resolved
packages/firestore/src/core/sync_engine_impl.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/bundle.test.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/snasphot_listener_source.test.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/snasphot_listener_source.test.ts Show resolved Hide resolved
packages/firestore/test/integration/api/snasphot_listener_source.test.ts Show resolved Hide resolved
packages/firestore/test/integration/api/snasphot_listener_source.test.ts Show resolved Hide resolved
packages/firestore/test/integration/api/snasphot_listener_source.test.ts Outdated Show resolved Hide resolved
@milaGGL milaGGL requested a review from wu-hui February 12, 2024 17:30
wu-hui
wu-hui reviewed Feb 13, 2024
View reviewed changes
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.

I am very happy with the thorough test coverage.

There is a comment about SnapshotInSync though, which I think is behavior is not expected. Can you investigate?

Other than this one, it should be good to go.

packages/firestore/test/unit/specs/listen_source_spec.test.ts Outdated Show resolved Hide resolved
packages/firestore/test/unit/specs/listen_source_spec.test.ts Show resolved Hide resolved
packages/firestore/test/unit/specs/listen_source_spec.test.ts Show resolved Hide resolved
packages/firestore/test/unit/specs/listen_source_spec.test.ts Outdated Show resolved Hide resolved
wu-hui
wu-hui approved these changes Feb 13, 2024
View reviewed changes
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.

LGTM, really good work, and sets an example of the test coverage we should be aiming for.

Thanks for your patience to address my feedbacks and questions.

@milaGGL milaGGL mentioned this pull request Feb 15, 2024
Merged
egilmorez
egilmorez approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Seconding Mark's approval, thanks!

@milaGGL milaGGL requested a review from markarndt March 8, 2024 15:27
@milaGGL
Copy link
Contributor Author

milaGGL commented Mar 8, 2024

@markarndt The public interface ListenSource has been changed from enum to union type, and the documentation has been changed a bit in format. Please help re-review the changed part.

markarndt
markarndt approved these changes Mar 8, 2024
View reviewed changes
docs-devsite/firestore_.md Outdated Show resolved Hide resolved
docs-devsite/firestore_.md Outdated Show resolved Hide resolved
@milaGGL milaGGL merged commit ce88e71 into master Mar 11, 2024
44 checks passed
@milaGGL milaGGL deleted the mila/snapshot-listener-source-from-cache branch March 11, 2024 19:08
@google-oss-bot google-oss-bot mentioned this pull request Mar 12, 2024
Merged
@firebase firebase locked and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@egilmorez egilmorez egilmorez approved these changes

@hsubox76 hsubox76 hsubox76 approved these changes

@markarndt markarndt markarndt approved these changes

@wu-hui wu-hui wu-hui approved these changes

@kevinthecheung kevinthecheung Awaiting requested review from kevinthecheung

Assignees

@wu-hui wu-hui

@milaGGL milaGGL

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@milaGGL @google-oss-bot @egilmorez @hsubox76 @markarndt @wu-hui @DellaBitta