From 629919ea760e35b7d880a099edf7f42b5bcbae4b Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Thu, 12 Sep 2024 13:54:24 -0400 Subject: [PATCH] Fix metadata sync issue for cache listeners in multi-tab (#8343) --- .changeset/warm-oranges-itch.md | 5 ++++ .../firestore/src/core/sync_engine_impl.ts | 8 +++--- .../src/local/shared_client_state.ts | 27 ++++++++++++++----- .../unit/specs/listen_source_spec.test.ts | 13 +++++---- 4 files changed, 37 insertions(+), 16 deletions(-) create mode 100644 .changeset/warm-oranges-itch.md diff --git a/.changeset/warm-oranges-itch.md b/.changeset/warm-oranges-itch.md new file mode 100644 index 00000000000..ee0c172090e --- /dev/null +++ b/.changeset/warm-oranges-itch.md @@ -0,0 +1,5 @@ +--- +'@firebase/firestore': patch +--- + +Fix an issue with metadata `fromCache` defaulting to `true` when listening to cache in multi-tabs. diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index 1f93db4f8bc..f96cbea0f00 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -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) { diff --git a/packages/firestore/src/local/shared_client_state.ts b/packages/firestore/src/local/shared_client_state.ts index 0d0e430c12f..7c033cedb41 100644 --- a/packages/firestore/src/local/shared_client_state.ts +++ b/packages/firestore/src/local/shared_client_state.ts @@ -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; @@ -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 @@ -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; } @@ -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'; } diff --git a/packages/firestore/test/unit/specs/listen_source_spec.test.ts b/packages/firestore/test/unit/specs/listen_source_spec.test.ts index ee2a9cf5944..3ebda23dbba 100644 --- a/packages/firestore/test/unit/specs/listen_source_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_source_spec.test.ts @@ -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) @@ -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) @@ -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) @@ -825,7 +825,7 @@ 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' }) @@ -833,8 +833,7 @@ describeSpec('Listens source options:', [], () => { .client(1) .expectEvents(query1, { hasPendingWrites: true, - added: [docB], - fromCache: true + added: [docB] }) .userUnlistensToCache(query1) );