fix(firestore): make sure that #docChanges() emits right event type when list shifts (#19)

Author: William Sedlacek

firestore/query-snapshot.test.ts

@@ -131,6 +131,33 @@ describe("#docChanges()", () => {
     });
   });
 
+  it("list docs as removed even when they are not the last in the list", () => {
+    const docs = [
+      new MockQueryDocumentSnapshot(query.firestore.doc("foo/0"), {}),
+      new MockQueryDocumentSnapshot(query.firestore.doc("foo/1"), {}),
+      new MockQueryDocumentSnapshot(query.firestore.doc("foo/2"), {}),
+      new MockQueryDocumentSnapshot(query.firestore.doc("foo/3"), {}),
+    ];
+
+    createSnapshot(docs);
+    const snapshot2 = createSnapshot([docs[0], docs[1], docs[3]]);
+
+    const changes = snapshot2.docChanges();
+    expect(changes).toHaveLength(2);
+    expect(changes).toContainEqual({
+      type: "removed",
+      oldIndex: 2,
+      newIndex: -1,
+      doc: docs[2],
+    });
+    expect(changes).toContainEqual({
+      type: "modified",
+      oldIndex: 3,
+      newIndex: 2,
+      doc: docs[3],
+    });
+  });
+
   it("lists docs as added when they are not in the previous snapshot", async () => {
     const docs = [
       new MockQueryDocumentSnapshot(query.firestore.doc("foo/bar"), {}),
@@ -151,6 +178,25 @@ describe("#docChanges()", () => {
   });
 });
 
+it("list changes that happen across different collections", () => {
+  const docs = [
+    new MockQueryDocumentSnapshot(query.firestore.doc("foo/a"), {}),
+    new MockQueryDocumentSnapshot(query.firestore.doc("bar/a"), {}),
+  ];
+
+  createSnapshot(docs);
+  const snapshot2 = createSnapshot([docs[0]]);
+
+  const changes = snapshot2.docChanges();
+  expect(changes).toHaveLength(1);
+  expect(changes).toContainEqual({
+    type: "removed",
+    oldIndex: 1,
+    newIndex: -1,
+    doc: docs[1],
+  });
+});
+
 describe("#forEach()", () => {
   it("iterates thru #docs", () => {
     const callback = jest.fn();

firestore/query-snapshot.ts

@@ -37,54 +37,43 @@ export class MockQuerySnapshot<T = firebase.firestore.DocumentData>
       }));
     }
 
-    const iterationSize = Math.max(this.previousSnapshot.size, this.size);
     const checkedPaths = new Set<string>();
     const previousDocs = this.previousSnapshot.docs;
     const changes: firebase.firestore.DocumentChange<T>[] = [];
-    for (let i = 0; i < iterationSize; i++) {
-      const doc = this.docs[i] || previousDocs[i];
-      if (checkedPaths.has(doc.ref.path)) {
-        // This is a document which has a lower index in the current snapshot.
-        // Skip it to avoid adding a modified change twice for it.
-        continue;
-      }
-
-      const newIndex =
-        doc === this.docs[i]
-          ? i
-          : this.docs.findIndex((another) => another.ref.path === doc.ref.path);
-      const oldIndex =
-        doc === previousDocs[i]
-          ? i
-          : previousDocs.findIndex((another) => another.ref.path === doc.ref.path);
 
-      if (oldIndex === -1) {
-        // Not in the old snapshot. Added.
+    for (const [newIndex, doc] of this.docs.entries()) {
+      const oldIndex = previousDocs.findIndex(({ ref }) => ref.path === doc.ref.path);
+      const previousDoc = previousDocs[oldIndex];
+      if (!previousDoc) {
         changes.push({
           type: "added",
           oldIndex,
           newIndex,
           doc,
         });
-      } else if (newIndex === -1) {
-        // Not in the new snapshot. Removed.
+      } else if (newIndex !== oldIndex || !previousDoc.isEqual(doc)) {
         changes.push({
-          type: "removed",
+          type: "modified",
           oldIndex,
           newIndex,
           doc,
         });
-      } else if (oldIndex !== newIndex || !previousDocs[oldIndex].isEqual(this.docs[newIndex])) {
-        // Different index or not the same content anymore? It's a change.
+      }
+
+      checkedPaths.add(doc.ref.path);
+    }
+
+    for (const [oldIndex, doc] of previousDocs.entries()) {
+      if (!checkedPaths.has(doc.ref.path)) {
         changes.push({
-          type: "modified",
+          type: "removed",
           oldIndex,
-          newIndex,
-          doc: this.docs[newIndex],
+          newIndex: -1,
+          doc,
         });
-      }
 
-      checkedPaths.add(doc.ref.path);
+        checkedPaths.add(doc.ref.path);
+      }
     }
 
     return changes;