Skip to content

Commit

Permalink
Fix metadata sync issue for cache listeners in multi-tab (#8343)
Browse files Browse the repository at this point in the history
  • Loading branch information
milaGGL authored Sep 12, 2024
1 parent 16d62d4 commit 629919e
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 16 deletions.
5 changes: 5 additions & 0 deletions .changeset/warm-oranges-itch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@firebase/firestore': patch
---

Fix an issue with metadata `fromCache` defaulting to `true` when listening to cache in multi-tabs.
8 changes: 5 additions & 3 deletions packages/firestore/src/core/sync_engine_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,9 +353,11 @@ async function allocateTargetAndMaybeListen(
// not registering it in shared client state, and directly calculate initial snapshots and
// subsequent updates from cache. Otherwise, register the target ID with local Firestore client
// as active watch target.
const status: QueryTargetState = shouldListenToRemote
? syncEngineImpl.sharedClientState.addLocalQueryTarget(targetId)
: 'not-current';
const status: QueryTargetState =
syncEngineImpl.sharedClientState.addLocalQueryTarget(
targetId,
/* addToActiveTargetIds= */ shouldListenToRemote
);

let viewSnapshot;
if (shouldInitializeView) {
Expand Down
27 changes: 21 additions & 6 deletions packages/firestore/src/local/shared_client_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,10 @@ export interface SharedClientState {
* If the target id is already associated with local client, the method simply
* returns its `QueryTargetState`.
*/
addLocalQueryTarget(targetId: TargetId): QueryTargetState;
addLocalQueryTarget(
targetId: TargetId,
addToActiveTargetIds?: boolean
): QueryTargetState;

/** Removes the Query Target ID association from the local client. */
removeLocalQueryTarget(targetId: TargetId): void;
Expand Down Expand Up @@ -655,7 +658,10 @@ export class WebStorageSharedClientState implements SharedClientState {
this.removeMutationState(batchId);
}

addLocalQueryTarget(targetId: TargetId): QueryTargetState {
addLocalQueryTarget(
targetId: TargetId,
addToActiveTargetIds: boolean = true
): QueryTargetState {
let queryState: QueryTargetState = 'not-current';

// Lookup an existing query state if the target ID was already registered
Expand All @@ -676,9 +682,13 @@ export class WebStorageSharedClientState implements SharedClientState {
}
}

this.localClientState.addQueryTarget(targetId);
this.persistClientState();
// If the query is listening to cache only, the target ID should not be registered with the
// local Firestore client as an active watch target.
if (addToActiveTargetIds) {
this.localClientState.addQueryTarget(targetId);
}

this.persistClientState();
return queryState;
}

Expand Down Expand Up @@ -1110,8 +1120,13 @@ export class MemorySharedClientState implements SharedClientState {
// No op.
}

addLocalQueryTarget(targetId: TargetId): QueryTargetState {
this.localState.addQueryTarget(targetId);
addLocalQueryTarget(
targetId: TargetId,
addToActiveTargetIds: boolean = true
): QueryTargetState {
if (addToActiveTargetIds) {
this.localState.addQueryTarget(targetId);
}
return this.queryState[targetId] || 'not-current';
}

Expand Down
13 changes: 6 additions & 7 deletions packages/firestore/test/unit/specs/listen_source_spec.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -491,10 +491,10 @@ describeSpec('Listens source options:', [], () => {
// Listen to cache in secondary clients
.client(1)
.userListensToCache(query1)
.expectEvents(query1, { added: [docA], fromCache: true })
.expectEvents(query1, { added: [docA] })
.client(2)
.userListensToCache(query1)
.expectEvents(query1, { added: [docA], fromCache: true })
.expectEvents(query1, { added: [docA] })
// Updates in the primary client notifies listeners sourcing from cache
// in secondary clients.
.client(0)
Expand Down Expand Up @@ -748,7 +748,7 @@ describeSpec('Listens source options:', [], () => {
//Listen to cache in secondary client
.client(1)
.userListensToCache(limitToLast)
.expectEvents(limitToLast, { added: [docB, docA], fromCache: true })
.expectEvents(limitToLast, { added: [docB, docA] })
// Watch sends document changes
.client(0)
.watchSends({ affects: [limit] }, docC)
Expand Down Expand Up @@ -794,7 +794,7 @@ describeSpec('Listens source options:', [], () => {
// Listen to cache in primary client
.client(0)
.userListensToCache(limitToLast)
.expectEvents(limitToLast, { added: [docB, docA], fromCache: true })
.expectEvents(limitToLast, { added: [docB, docA] })
// Watch sends document changes
.watchSends({ affects: [limit] }, docC)
.watchSnapshots(2000)
Expand Down Expand Up @@ -825,16 +825,15 @@ describeSpec('Listens source options:', [], () => {
// Listen to cache in secondary client
.client(1)
.userListensToCache(query1)
.expectEvents(query1, { added: [docA], fromCache: true })
.expectEvents(query1, { added: [docA] })
.client(0)
.userUnlistens(query1)
.userSets('collection/b', { key: 'b' })
// The other listener should work as expected
.client(1)
.expectEvents(query1, {
hasPendingWrites: true,
added: [docB],
fromCache: true
added: [docB]
})
.userUnlistensToCache(query1)
);
Expand Down

0 comments on commit 629919e

Please sign in to comment.