From f4d0654223f3350fcd964dfcc9284584a404b03e Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 28 Jun 2024 11:54:54 -0400 Subject: [PATCH 1/3] fix QueryTargetState update logic --- .../firestore/src/core/sync_engine_impl.ts | 8 +++--- .../src/local/shared_client_state.ts | 25 ++++++++++++++----- .../unit/specs/listen_source_spec.test.ts | 13 +++++----- 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index bba07f4f4bc..fa40f302bd8 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, + 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..dd7f2b465ef 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, + addActiveTargetId?: 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, + addActiveTargetId: boolean = true + ): QueryTargetState { let queryState: QueryTargetState = 'not-current'; // Lookup an existing query state if the target ID was already registered @@ -676,8 +682,10 @@ export class WebStorageSharedClientState implements SharedClientState { } } - this.localClientState.addQueryTarget(targetId); - this.persistClientState(); + if (addActiveTargetId) { + this.localClientState.addQueryTarget(targetId); + this.persistClientState(); + } return queryState; } @@ -1110,8 +1118,13 @@ export class MemorySharedClientState implements SharedClientState { // No op. } - addLocalQueryTarget(targetId: TargetId): QueryTargetState { - this.localState.addQueryTarget(targetId); + addLocalQueryTarget( + targetId: TargetId, + addActiveTargetId: boolean = true + ): QueryTargetState { + if (addActiveTargetId) { + 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) ); From a3eadb4be3febc9632137c5e96d9bd7c24007d8c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 28 Jun 2024 13:15:10 -0400 Subject: [PATCH 2/3] add changeset, rename parameter --- .changeset/warm-oranges-itch.md | 5 +++++ packages/firestore/src/core/sync_engine_impl.ts | 2 +- .../firestore/src/local/shared_client_state.ts | 14 ++++++++------ 3 files changed, 14 insertions(+), 7 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..3999ebd874d --- /dev/null +++ b/.changeset/warm-oranges-itch.md @@ -0,0 +1,5 @@ +--- +'@firebase/firestore': patch +--- + +Fix metadata `fromCache` default to `true` issue when listen 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 fa40f302bd8..457fdf6f2bd 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -356,7 +356,7 @@ async function allocateTargetAndMaybeListen( const status: QueryTargetState = syncEngineImpl.sharedClientState.addLocalQueryTarget( targetId, - shouldListenToRemote + /* addToActiveTargetIds= */ shouldListenToRemote ); let viewSnapshot; diff --git a/packages/firestore/src/local/shared_client_state.ts b/packages/firestore/src/local/shared_client_state.ts index dd7f2b465ef..cc1738b855d 100644 --- a/packages/firestore/src/local/shared_client_state.ts +++ b/packages/firestore/src/local/shared_client_state.ts @@ -106,7 +106,7 @@ export interface SharedClientState { */ addLocalQueryTarget( targetId: TargetId, - addActiveTargetId?: boolean + addToActiveTargetIds?: boolean ): QueryTargetState; /** Removes the Query Target ID association from the local client. */ @@ -660,7 +660,7 @@ export class WebStorageSharedClientState implements SharedClientState { addLocalQueryTarget( targetId: TargetId, - addActiveTargetId: boolean = true + addToActiveTargetIds: boolean = true ): QueryTargetState { let queryState: QueryTargetState = 'not-current'; @@ -682,11 +682,13 @@ export class WebStorageSharedClientState implements SharedClientState { } } - if (addActiveTargetId) { + // If the query is listening to cache only, the target ID should not be registered the with + // local Firestore client as active watch target. + if (addToActiveTargetIds) { this.localClientState.addQueryTarget(targetId); - this.persistClientState(); } + this.persistClientState(); return queryState; } @@ -1120,9 +1122,9 @@ export class MemorySharedClientState implements SharedClientState { addLocalQueryTarget( targetId: TargetId, - addActiveTargetId: boolean = true + addToActiveTargetIds: boolean = true ): QueryTargetState { - if (addActiveTargetId) { + if (addToActiveTargetIds) { this.localState.addQueryTarget(targetId); } return this.queryState[targetId] || 'not-current'; From 6c3ef5bf967a0a06702120295a74838898283e00 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 2 Jul 2024 11:53:52 -0400 Subject: [PATCH 3/3] update the comments --- .changeset/warm-oranges-itch.md | 2 +- packages/firestore/src/local/shared_client_state.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changeset/warm-oranges-itch.md b/.changeset/warm-oranges-itch.md index 3999ebd874d..ee0c172090e 100644 --- a/.changeset/warm-oranges-itch.md +++ b/.changeset/warm-oranges-itch.md @@ -2,4 +2,4 @@ '@firebase/firestore': patch --- -Fix metadata `fromCache` default to `true` issue when listen to cache in multi-tabs. +Fix an issue with metadata `fromCache` defaulting to `true` when listening to cache in multi-tabs. diff --git a/packages/firestore/src/local/shared_client_state.ts b/packages/firestore/src/local/shared_client_state.ts index cc1738b855d..7c033cedb41 100644 --- a/packages/firestore/src/local/shared_client_state.ts +++ b/packages/firestore/src/local/shared_client_state.ts @@ -682,8 +682,8 @@ export class WebStorageSharedClientState implements SharedClientState { } } - // If the query is listening to cache only, the target ID should not be registered the with - // local Firestore client as active watch target. + // 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); }