Don't look up TargetId in notifyLocalViewChanges (#1065)

Author: schmidt-sebastian

packages/firestore/src/core/sync_engine.ts

@@ -598,6 +598,7 @@ export class SyncEngine implements RemoteSyncer {
               if (viewChange.snapshot) {
                 newSnaps.push(viewChange.snapshot);
                 const docChanges = LocalViewChanges.fromSnapshot(
+                  queryView.targetId,
                   viewChange.snapshot
                 );
                 docChangesInAllViews.push(docChanges);
@@ -607,14 +608,11 @@ export class SyncEngine implements RemoteSyncer {
       );
     });
 
-    return Promise.all(queriesProcessed)
-      .then(() => {
-        this.viewHandler!(newSnaps);
-        return this.localStore.notifyLocalViewChanges(docChangesInAllViews);
-      })
-      .then(() => {
-        return this.localStore.collectGarbage();
-      });
+    return Promise.all(queriesProcessed).then(() => {
+      this.viewHandler!(newSnaps);
+      this.localStore.notifyLocalViewChanges(docChangesInAllViews);
+      return this.localStore.collectGarbage();
+    });
   }
 
   private assertSubscribed(fnName: string): void {

packages/firestore/src/local/local_store.ts

@@ -553,29 +553,17 @@ export class LocalStore {
   /**
    * Notify local store of the changed views to locally pin documents.
    */
-  notifyLocalViewChanges(viewChanges: LocalViewChanges[]): Promise<void> {
-    return this.persistence.runTransaction('Notify local view changes', txn => {
-      const promises = [] as Array<PersistencePromise<void>>;
-      for (const view of viewChanges) {
-        promises.push(
-          this.queryCache
-            .getQueryData(txn, view.query)
-            .next((queryData: QueryData | null) => {
-              assert(
-                queryData !== null,
-                'Local view changes contain unallocated query.'
-              );
-              const targetId = queryData!.targetId;
-              this.localViewReferences.addReferences(view.addedKeys, targetId);
-              this.localViewReferences.removeReferences(
-                view.removedKeys,
-                targetId
-              );
-            })
-        );
-      }
-      return PersistencePromise.waitFor(promises);
-    });
+  notifyLocalViewChanges(viewChanges: LocalViewChanges[]): void {
+    for (const viewChange of viewChanges) {
+      this.localViewReferences.addReferences(
+        viewChange.addedKeys,
+        viewChange.targetId
+      );
+      this.localViewReferences.removeReferences(
+        viewChange.removedKeys,
+        viewChange.targetId
+      );
+    }
   }
 
   /**

packages/firestore/src/local/local_view_changes.ts

@@ -14,9 +14,9 @@
  * limitations under the License.
  */
 
-import { Query } from '../core/query';
 import { ChangeType, ViewSnapshot } from '../core/view_snapshot';
 import { documentKeySet, DocumentKeySet } from '../model/collections';
+import { TargetId } from '../core/types';
 
 /**
  * A set of changes to what documents are currently in view and out of view for
@@ -25,12 +25,15 @@ import { documentKeySet, DocumentKeySet } from '../model/collections';
  */
 export class LocalViewChanges {
   constructor(
-    readonly query: Query,
+    readonly targetId: TargetId,
     readonly addedKeys: DocumentKeySet,
     readonly removedKeys: DocumentKeySet
   ) {}
 
-  static fromSnapshot(viewSnapshot: ViewSnapshot): LocalViewChanges {
+  static fromSnapshot(
+    targetId: TargetId,
+    viewSnapshot: ViewSnapshot
+  ): LocalViewChanges {
     let addedKeys = documentKeySet();
     let removedKeys = documentKeySet();
 
@@ -47,6 +50,6 @@ export class LocalViewChanges {
       }
     }
 
-    return new LocalViewChanges(viewSnapshot.query, addedKeys, removedKeys);
+    return new LocalViewChanges(targetId, addedKeys, removedKeys);
   }
 }

packages/firestore/test/unit/local/local_store.test.ts

@@ -115,9 +115,9 @@ class LocalStoreTester {
   }
 
   afterViewChanges(viewChanges: LocalViewChanges): LocalStoreTester {
-    this.promiseChain = this.promiseChain.then(() => {
-      return this.localStore.notifyLocalViewChanges([viewChanges]);
-    });
+    this.promiseChain = this.promiseChain.then(() =>
+      this.localStore.notifyLocalViewChanges([viewChanges])
+    );
     return this;
   }
 
@@ -786,14 +786,14 @@ function genericLocalStoreTests(
       .afterGC()
       .toContain(doc('foo/bar', 1, { foo: 'bar' }))
       .toContain(doc('foo/baz', 0, { foo: 'baz' }, { hasLocalMutations: true }))
-      .after(localViewChanges(query, { added: ['foo/bar', 'foo/baz'] }))
+      .after(localViewChanges(2, { added: ['foo/bar', 'foo/baz'] }))
       .after(docUpdateRemoteEvent(doc('foo/bar', 1, { foo: 'bar' }), [], [2]))
       .after(docUpdateRemoteEvent(doc('foo/baz', 2, { foo: 'baz' }), [2]))
       .afterAcknowledgingMutation({ documentVersion: 2 })
       .afterGC()
       .toContain(doc('foo/bar', 1, { foo: 'bar' }))
       .toContain(doc('foo/baz', 2, { foo: 'baz' }))
-      .after(localViewChanges(query, { removed: ['foo/bar', 'foo/baz'] }))
+      .after(localViewChanges(2, { removed: ['foo/bar', 'foo/baz'] }))
       .afterReleasingQuery(query)
       .afterGC()
       .toNotContain('foo/bar')

packages/firestore/test/util/helpers.ts

@@ -30,7 +30,6 @@ import {
   Direction,
   Filter,
   OrderBy,
-  Query,
   RelationOp
 } from '../../src/core/query';
 import { SnapshotVersion } from '../../src/core/snapshot_version';
@@ -371,7 +370,7 @@ export function limboChanges(changes: {
 }
 
 export function localViewChanges(
-  query: Query,
+  targetId: TargetId,
   changes: { added?: string[]; removed?: string[] }
 ): LocalViewChanges {
   if (!changes.added) changes.added = [];
@@ -386,7 +385,7 @@ export function localViewChanges(
     keyStr => (removedKeys = removedKeys.add(key(keyStr)))
   );
 
-  return new LocalViewChanges(query, addedKeys, removedKeys);
+  return new LocalViewChanges(targetId, addedKeys, removedKeys);
 }
 
 /** Creates a resume token to match the given snapshot version. */