fix(afs): fixing the metadata in snapshotChanges and more (#2670)

* metadata updates weren't getting to the `snapshotChanges` on collections due to `docChanges()` not including metadata in the changes, even when you ask for it
* pull `spliceAndSlice` from rxfire and use it in combination with `distinctUntilChanged()` to reduce unneeded change detection cycles

Author: jamesdaniels

sample/src/app/app.module.ts

@@ -37,6 +37,7 @@ import { MessagingComponent } from './messaging/messaging.component';
 import { FunctionsComponent } from './functions/functions.component';
 import { FirestoreOfflineComponent } from './firestore-offline/firestore-offline.component';
 import { FirestoreOfflineModule } from './firestore-offline/firestore-offline.module';
+import { UpboatsComponent } from './upboats/upboats.component';
 
 @NgModule({
   declarations: [
@@ -50,6 +51,7 @@ import { FirestoreOfflineModule } from './firestore-offline/firestore-offline.mo
     AuthComponent,
     MessagingComponent,
     FunctionsComponent,
+    UpboatsComponent,
   ],
   imports: [
     BrowserModule.withServerTransition({ appId: 'serverApp' }),

sample/src/app/home/home.component.ts

@@ -9,6 +9,7 @@ import { FirebaseApp } from '@angular/fire';
     <app-database></app-database>
     <app-firestore></app-firestore>
     <app-firestore-offline></app-firestore-offline>
+    <app-upboats></app-upboats>
     <app-storage></app-storage>
     <app-auth></app-auth>
     <app-remote-config></app-remote-config>

sample/src/app/protected-lazy/protected-lazy.component.html

@@ -1 +1,9 @@
 <p>protected-lazy works!</p>
+
+<ul>
+    <li *ngFor="let item of snapshot | async">
+        <pre>data(): {{ item.payload.doc.data() | json }},
+id: {{ item.payload.doc.id }},
+metadata: {{ item.payload.doc.metadata | json }}</pre>
+    </li>
+</ul>

sample/src/app/protected-lazy/protected-lazy.component.ts

@@ -1,4 +1,7 @@
 import { Component, OnInit } from '@angular/core';
+import { DocumentChangeAction } from '@angular/fire/firestore';
+import { Observable } from 'rxjs';
+import { AngularFirestoreOffline } from '../firestore-offline/firestore-offline.module';
 
 @Component({
   selector: 'app-protected-lazy',
@@ -7,7 +10,11 @@ import { Component, OnInit } from '@angular/core';
 })
 export class ProtectedLazyComponent implements OnInit {
 
-  constructor() { }
+  public snapshot: Observable<DocumentChangeAction<unknown>[]>;
+
+  constructor(private afs: AngularFirestoreOffline) {
+    this.snapshot = afs.collection('test').snapshotChanges();
+  }
 
   ngOnInit(): void {
   }

sample/src/app/upboats/upboats.component.css

@@ -0,0 +1,11 @@
+<ul>
+    <li *ngFor="let animal of animals | async">
+        <span>{{ animal.name }}</span>
+        <button (click)="upboat(animal.id)">👍</button>
+        <span>{{ animal.upboats }}</span>
+        <button (click)="downboat(animal.id)">👎</button>
+        <span *ngIf="animal.hasPendingWrites">🕒</span>
+    </li>
+</ul>
+
+<button (click)="newAnimal()">New animal</button>

sample/src/app/upboats/upboats.component.html

@@ -0,0 +1,25 @@
+import { ComponentFixture, TestBed } from '@angular/core/testing';
+
+import { UpboatsComponent } from './upboats.component';
+
+describe('UpboatsComponent', () => {
+  let component: UpboatsComponent;
+  let fixture: ComponentFixture<UpboatsComponent>;
+
+  beforeEach(async () => {
+    await TestBed.configureTestingModule({
+      declarations: [ UpboatsComponent ]
+    })
+    .compileComponents();
+  });
+
+  beforeEach(() => {
+    fixture = TestBed.createComponent(UpboatsComponent);
+    component = fixture.componentInstance;
+    fixture.detectChanges();
+  });
+
+  it('should create', () => {
+    expect(component).toBeTruthy();
+  });
+});

sample/src/app/upboats/upboats.component.spec.ts

@@ -0,0 +1,65 @@
+import { Component, OnInit } from '@angular/core';
+import { Observable } from 'rxjs';
+import { map, startWith, tap } from 'rxjs/operators';
+import { AngularFirestoreOffline } from '../firestore-offline/firestore-offline.module';
+import firebase from 'firebase/app';
+import { makeStateKey, TransferState } from '@angular/platform-browser';
+import { trace } from '@angular/fire/performance';
+
+type Animal = { name: string, upboats: number, id: string, hasPendingWrites: boolean };
+
+@Component({
+  selector: 'app-upboats',
+  templateUrl: './upboats.component.html',
+  styleUrls: ['./upboats.component.css']
+})
+export class UpboatsComponent implements OnInit {
+
+  public animals: Observable<Animal[]>;
+
+  constructor(private firestore: AngularFirestoreOffline, state: TransferState) {
+    const collection = firestore.collection<Animal>('animals', ref =>
+      ref.orderBy('upboats', 'desc').orderBy('updatedAt', 'desc')
+    );
+    const key = makeStateKey(collection.ref.path);
+    const existing = state.get(key, undefined);
+    this.animals = collection.snapshotChanges().pipe(
+      trace('animals'),
+      map(it => it.map(change => ({
+        ...change.payload.doc.data(),
+        id: change.payload.doc.id,
+        hasPendingWrites: change.payload.doc.metadata.hasPendingWrites
+      }))),
+      existing ? startWith(existing) : tap(it => state.set(key, it))
+    );
+  }
+
+  ngOnInit(): void {
+  }
+
+  upboat(id: string) {
+    // TODO add rule
+    this.firestore.doc(`animals/${id}`).update({
+      upboats: firebase.firestore.FieldValue.increment(1),
+      updatedAt: firebase.firestore.FieldValue.serverTimestamp(),
+    });
+  }
+
+  downboat(id: string) {
+    // TODO add rule
+    this.firestore.doc(`animals/${id}`).update({
+      upboats: firebase.firestore.FieldValue.increment(-1),
+      updatedAt: firebase.firestore.FieldValue.serverTimestamp(),
+    });
+  }
+
+  newAnimal() {
+    // TODO add rule
+    this.firestore.collection('animals').add({
+      name: prompt('Can haz name?'),
+      upboats: 1,
+      updatedAt: firebase.firestore.FieldValue.serverTimestamp(),
+    });
+  }
+
+}

sample/src/app/upboats/upboats.component.ts

@@ -1,6 +1,6 @@
 import { fromCollectionRef } from '../observable/fromRef';
 import { Observable, SchedulerLike } from 'rxjs';
-import { map, scan } from 'rxjs/operators';
+import { distinctUntilChanged, map, pairwise, scan, startWith } from 'rxjs/operators';
 
 import { DocumentChange, DocumentChangeAction, DocumentChangeType, Query } from '../interfaces';
 
@@ -25,8 +25,29 @@ export function sortedChanges<T>(
   scheduler?: SchedulerLike): Observable<DocumentChangeAction<T>[]> {
   return fromCollectionRef(query, scheduler)
     .pipe(
-      map(changes => changes.payload.docChanges()),
-      scan((current, changes) => combineChanges(current, changes, events), []),
+      startWith(undefined),
+      pairwise(),
+      scan((current, [priorChanges, changes]) => {
+        const docChanges = changes.payload.docChanges();
+        const ret = combineChanges(current, docChanges, events);
+        // docChanges({ includeMetadataChanges: true }) does't include metadata changes... wat?
+        if (events.indexOf('modified') > -1 && priorChanges &&
+            JSON.stringify(priorChanges.payload.metadata) !== JSON.stringify(changes.payload.metadata)) {
+          return ret.map(it => {
+            const partOfDocChanges = !!docChanges.find(d => d.doc.ref.isEqual(it.doc.ref));
+            return {
+              // if it's not one of the changed docs that means we already saw it's order change
+              // so this is purely metadata, so don't move the doc
+              oldIndex: partOfDocChanges ? it.oldIndex : it.newIndex,
+              newIndex: it.newIndex,
+              type: 'modified',
+              doc: changes.payload.docs.find(d => d.ref.isEqual(it.doc.ref))
+            };
+          });
+        }
+        return ret;
+      }, []),
+      distinctUntilChanged(), // cut down on unneed change cycles
       map(changes => changes.map(c => ({ type: c.type, payload: c } as DocumentChangeAction<T>))));
 }
 
@@ -44,6 +65,21 @@ export function combineChanges<T>(current: DocumentChange<T>[], changes: Documen
   return current;
 }
 
+/**
+ * Splice arguments on top of a sliced array, to break top-level ===
+ * this is useful for change-detection
+ */
+function sliceAndSplice<T>(
+  original: T[],
+  start: number,
+  deleteCount: number,
+  ...args: T[]
+): T[] {
+  const returnArray = original.slice();
+  returnArray.splice(start, deleteCount, ...args);
+  return returnArray;
+}
+
 /**
  * Creates a new sorted array from a new change.
  */
@@ -53,24 +89,26 @@ export function combineChange<T>(combined: DocumentChange<T>[], change: Document
       if (combined[change.newIndex] && combined[change.newIndex].doc.ref.isEqual(change.doc.ref)) {
         // Not sure why the duplicates are getting fired
       } else {
-        combined.splice(change.newIndex, 0, change);
+        return sliceAndSplice(combined, change.newIndex, 0, change);
       }
       break;
     case 'modified':
       if (combined[change.oldIndex] == null || combined[change.oldIndex].doc.ref.isEqual(change.doc.ref)) {
         // When an item changes position we first remove it
         // and then add it's new position
         if (change.oldIndex !== change.newIndex) {
-          combined.splice(change.oldIndex, 1);
-          combined.splice(change.newIndex, 0, change);
+          const copiedArray = combined.slice();
+          copiedArray.splice(change.oldIndex, 1);
+          copiedArray.splice(change.newIndex, 0, change);
+          return copiedArray;
         } else {
-          combined.splice(change.newIndex, 1, change);
+          return sliceAndSplice(combined, change.newIndex, 1, change);
         }
       }
       break;
     case 'removed':
       if (combined[change.oldIndex] && combined[change.oldIndex].doc.ref.isEqual(change.doc.ref)) {
-        combined.splice(change.oldIndex, 1);
+        return sliceAndSplice(combined, change.oldIndex, 1);
       }
       break;
   }