Review

Author: schmidt-sebastian

packages/firestore/src/index/index_entry.ts

@@ -52,7 +52,7 @@ export function indexEntryComparator(
 function compareByteArrays(left: Uint8Array, right: Uint8Array): number {
   for (let i = 0; i < left.length && i < right.length; ++i) {
     const compare = left[i] - right[i];
-    if (compare) {
+    if (compare !== 0) {
       return compare;
     }
   }

packages/firestore/src/local/indexeddb_index_manager.ts

@@ -320,6 +320,10 @@ export class IndexedDbIndexManager implements IndexManager {
     transaction: PersistenceTransaction,
     documents: DocumentMap
   ): PersistencePromise<void> {
+    // Porting Note: `getFieldIndexes()` on Web does not cache index lookups as
+    // it could be used across different IndexedDB transactions. As any cached
+    // data might invalidated by other multi-tab clients, we can only trust data
+    // within a single IndexedDB transaction. We therefore add a cache here.
     const memoizedIndexes = new Map<string, FieldIndex[]>();
     return PersistencePromise.forEach(documents, (key, doc) => {
       const memoizedCollectionIndexes = memoizedIndexes.get(
@@ -481,10 +485,10 @@ export class IndexedDbIndexManager implements IndexManager {
       existingEntries,
       newEntries,
       indexEntryComparator,
-      entry => {
+      /* onAdd= */ entry => {
         promises.push(this.addIndexEntry(transaction, document, entry));
       },
-      entry => {
+      /* onRemove= */ entry => {
         promises.push(this.deleteIndexEntry(transaction, document, entry));
       }
     );

packages/firestore/test/unit/util/sorted_set.test.ts

@@ -158,20 +158,39 @@ describe('SortedSet', () => {
   });
 
   it('diff sorted sets with missing element', () => {
-    validateDiffSortedSets(['a', 'b', 'c'], ['a', 'b']);
+    const { added, removed } = computeDiffs(['a', 'b', 'c'], ['a', 'b']);
+    expect(added).to.have.members([]);
+    expect(removed).to.have.members(['c']);
   });
 
   it('diff sorted sets with added element', () => {
-    validateDiffSortedSets(['a', 'b'], ['a', 'b', 'c']);
+    const { added, removed } = computeDiffs(['a', 'b'], ['a', 'b', 'c']);
+    expect(added).to.have.members(['c']);
+    expect(removed).to.have.members([]);
   });
 
   it('diff sorted sets with empty sets', () => {
-    validateDiffSortedSets(['a'], []);
-    validateDiffSortedSets([], ['a']);
-    validateDiffSortedSets([], []);
+    {
+      let { added, removed } = computeDiffs(['a'], []);
+      expect(added).to.have.members([]);
+      expect(removed).to.have.members(['a']);
+    }
+    {
+      let { added, removed } = computeDiffs([], ['a']);
+      expect(added).to.have.members(['a']);
+      expect(removed).to.have.members([]);
+    }
+    {
+      const { added, removed } = computeDiffs([], []);
+      expect(added).to.have.members([]);
+      expect(removed).to.have.members([]);
+    }
   });
 
-  function validateDiffSortedSets(before: string[], after: string[]): void {
+  function computeDiffs(
+    before: string[],
+    after: string[]
+  ): { added: string[]; removed: string[] } {
     let beforeSorted = new SortedSet<string>(primitiveComparator);
     let afterSorted = new SortedSet<string>(primitiveComparator);
     before.forEach(v => {
@@ -181,18 +200,19 @@ describe('SortedSet', () => {
       afterSorted = afterSorted.add(v);
     });
 
-    let result = beforeSorted;
+    const added: string[] = [];
+    const removed: string[] = [];
     diffSortedSets(
       beforeSorted,
       afterSorted,
       primitiveComparator,
       v => {
-        result = result.add(v);
+        added.push(v);
       },
       v => {
-        result = result.delete(v);
+        removed.push(v);
       }
     );
-    expect(result.isEqual(afterSorted)).to.be.true;
+    return { added, removed };
   }
 });