From dab62156145dfa8c24fe9edec440bda9ba1ae022 Mon Sep 17 00:00:00 2001 From: James Daniels Date: Tue, 24 Nov 2020 01:42:08 -0500 Subject: [PATCH 1/3] fix(afs): snapshotChanges and auditLog now have metadata changes --- src/firestore/collection/changes.ts | 106 +++++++++++++++---------- src/firestore/collection/collection.ts | 1 + 2 files changed, 65 insertions(+), 42 deletions(-) diff --git a/src/firestore/collection/changes.ts b/src/firestore/collection/changes.ts index 080bff5d0..9e3f4de10 100644 --- a/src/firestore/collection/changes.ts +++ b/src/firestore/collection/changes.ts @@ -1,6 +1,6 @@ import { fromCollectionRef } from '../observable/fromRef'; -import { Observable, SchedulerLike } from 'rxjs'; -import { distinctUntilChanged, map, pairwise, scan, startWith } from 'rxjs/operators'; +import { Observable, of, SchedulerLike } from 'rxjs'; +import { concatMap, distinctUntilChanged, map, pairwise, scan, startWith } from 'rxjs/operators'; import { DocumentChange, DocumentChangeAction, DocumentChangeType, Query } from '../interfaces'; @@ -11,9 +11,48 @@ import { DocumentChange, DocumentChangeAction, DocumentChangeType, Query } from export function docChanges(query: Query, scheduler?: SchedulerLike): Observable[]> { return fromCollectionRef(query, scheduler) .pipe( - map(action => - action.payload.docChanges() - .map(change => ({ type: change.type, payload: change } as DocumentChangeAction)))); + startWith(undefined), + pairwise(), + map(([priorAction, action]) => { + const docChanges = action.payload.docChanges(); + // if it's the first fire from the cache it's not added + const overrideAsModified = !priorAction && action.payload.metadata.fromCache; + const ret = docChanges.map(change => { + if (overrideAsModified) { + return { + type: 'modified', payload: { + oldIndex: change.oldIndex, + newIndex: change.newIndex, + type: 'modified', + doc: change.doc, + } + }; + } else { + return { type: change.type, payload: change }; + } + }); + if (priorAction && JSON.stringify(priorAction.payload.metadata) !== JSON.stringify(action.payload.metadata)) { + return [ret, action.payload.docs.map((it, i) => { + const docChange = docChanges.find(d => d.doc.ref.isEqual(it.ref)); + const priorDoc = priorAction?.payload.docs.find(d => d.ref.isEqual(it.ref)); + if (!docChange && priorDoc && JSON.stringify(priorDoc.metadata) === JSON.stringify(it.metadata)) { + return null; + } + return { + type: 'modified', + payload: { + oldIndex: i, + newIndex: i, + type: 'modified', + doc: it + } + } as DocumentChangeAction; + }).filter(it => !!it)]; + } + return [ret as DocumentChangeAction[]]; + }), + concatMap(it => of(...it) as Observable[]>), + ); } /** @@ -23,30 +62,9 @@ export function sortedChanges( query: Query, events: DocumentChangeType[], scheduler?: SchedulerLike): Observable[]> { - return fromCollectionRef(query, scheduler) + return docChanges(query, scheduler) .pipe( - 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; - }, []), + scan((current, changes) => combineChanges(current, changes.map(it => it.payload), events), []), distinctUntilChanged(), // cut down on unneed change cycles map(changes => changes.map(c => ({ type: c.type, payload: c } as DocumentChangeAction)))); } @@ -85,6 +103,24 @@ function sliceAndSplice( */ export function combineChange(combined: DocumentChange[], change: DocumentChange): DocumentChange[] { switch (change.type) { + case 'modified': + // OldIndex == -1 is added, since we added modified if fromCache, skip over, don't break + if (change.oldIndex > -1) { + 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) { + const copiedArray = combined.slice(); + copiedArray.splice(change.oldIndex, 1); + copiedArray.splice(change.newIndex, 0, change); + return copiedArray; + } else { + return sliceAndSplice(combined, change.newIndex, 1, change); + } + } + break; + } + // tslint:disable-next-line:no-switch-case-fall-through case 'added': if (combined[change.newIndex] && combined[change.newIndex].doc.ref.isEqual(change.doc.ref)) { // Not sure why the duplicates are getting fired @@ -92,20 +128,6 @@ export function combineChange(combined: DocumentChange[], change: Document 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) { - const copiedArray = combined.slice(); - copiedArray.splice(change.oldIndex, 1); - copiedArray.splice(change.newIndex, 0, change); - return copiedArray; - } else { - return sliceAndSplice(combined, change.newIndex, 1, change); - } - } - break; case 'removed': if (combined[change.oldIndex] && combined[change.oldIndex].doc.ref.isEqual(change.doc.ref)) { return sliceAndSplice(combined, change.oldIndex, 1); diff --git a/src/firestore/collection/collection.ts b/src/firestore/collection/collection.ts index 9e77f27a0..07c4e2131 100644 --- a/src/firestore/collection/collection.ts +++ b/src/firestore/collection/collection.ts @@ -61,6 +61,7 @@ export class AngularFirestoreCollection { stateChanges(events?: DocumentChangeType[]): Observable[]> { if (!events || events.length === 0) { return docChanges(this.query, this.afs.schedulers.outsideAngular).pipe( + filter(changes => changes.length > 0), this.afs.keepUnstableUntilFirst ); } From 72fc72b9752f2edab4b2a7633342fac137fa1ee9 Mon Sep 17 00:00:00 2001 From: James Daniels Date: Tue, 24 Nov 2020 12:21:36 -0500 Subject: [PATCH 2/3] Cleanup --- sample/src/app/app.component.ts | 3 +- src/firestore/collection/changes.ts | 97 +++++++++++++---------------- 2 files changed, 45 insertions(+), 55 deletions(-) diff --git a/sample/src/app/app.component.ts b/sample/src/app/app.component.ts index 9b6ffc97c..90409f892 100644 --- a/sample/src/app/app.component.ts +++ b/sample/src/app/app.component.ts @@ -1,5 +1,6 @@ import { ApplicationRef, Component } from '@angular/core'; import { FirebaseApp } from '@angular/fire'; +import { debounceTime } from 'rxjs/operators'; @Component({ selector: 'app-root', @@ -25,6 +26,6 @@ import { FirebaseApp } from '@angular/fire'; }) export class AppComponent { constructor(public readonly firebaseApp: FirebaseApp, appRef: ApplicationRef) { - appRef.isStable.subscribe(it => console.log('isStable', it)); + appRef.isStable.pipe(debounceTime(200)).subscribe(it => console.log('isStable', it)); } } diff --git a/src/firestore/collection/changes.ts b/src/firestore/collection/changes.ts index 9e3f4de10..8ae7cd357 100644 --- a/src/firestore/collection/changes.ts +++ b/src/firestore/collection/changes.ts @@ -1,7 +1,6 @@ import { fromCollectionRef } from '../observable/fromRef'; -import { Observable, of, SchedulerLike } from 'rxjs'; -import { concatMap, distinctUntilChanged, map, pairwise, scan, startWith } from 'rxjs/operators'; - +import { Observable, SchedulerLike } from 'rxjs'; +import { bufferTime, distinctUntilChanged, map, pairwise, scan, startWith } from 'rxjs/operators'; import { DocumentChange, DocumentChangeAction, DocumentChangeType, Query } from '../interfaces'; /** @@ -15,43 +14,35 @@ export function docChanges(query: Query, scheduler?: SchedulerLike): Observab pairwise(), map(([priorAction, action]) => { const docChanges = action.payload.docChanges(); - // if it's the first fire from the cache it's not added - const overrideAsModified = !priorAction && action.payload.metadata.fromCache; - const ret = docChanges.map(change => { - if (overrideAsModified) { - return { - type: 'modified', payload: { - oldIndex: change.oldIndex, - newIndex: change.newIndex, - type: 'modified', - doc: change.doc, - } - }; - } else { - return { type: change.type, payload: change }; - } - }); + const actions = docChanges.map(change => ({ type: change.type, payload: change })); + // the metadata has changed from the prior emission if (priorAction && JSON.stringify(priorAction.payload.metadata) !== JSON.stringify(action.payload.metadata)) { - return [ret, action.payload.docs.map((it, i) => { - const docChange = docChanges.find(d => d.doc.ref.isEqual(it.ref)); - const priorDoc = priorAction?.payload.docs.find(d => d.ref.isEqual(it.ref)); - if (!docChange && priorDoc && JSON.stringify(priorDoc.metadata) === JSON.stringify(it.metadata)) { - return null; - } - return { - type: 'modified', - payload: { - oldIndex: i, - newIndex: i, + // go through all the docs in payload and figure out which ones changed + action.payload.docs.forEach((currentDoc, currentIndex) => { + const docChange = docChanges.find(d => d.doc.ref.isEqual(currentDoc.ref)); + const priorDoc = priorAction?.payload.docs.find(d => d.ref.isEqual(currentDoc.ref)); + if (docChange && JSON.stringify(docChange.doc.metadata) === JSON.stringify(currentDoc.metadata) || + !docChange && priorDoc && JSON.stringify(priorDoc.metadata) === JSON.stringify(currentDoc.metadata)) { + // document doesn't appear to have changed, don't log another action + } else { + // since the actions are processed in order just push onto the array + actions.push({ type: 'modified', - doc: it - } - } as DocumentChangeAction; - }).filter(it => !!it)]; + payload: { + oldIndex: currentIndex, + newIndex: currentIndex, + type: 'modified', + doc: currentDoc + } + }); + } + }); } - return [ret as DocumentChangeAction[]]; + return actions as DocumentChangeAction[]; }), - concatMap(it => of(...it) as Observable[]>), + // there are some sync changes firing, esp. when taking over from cache, buffer and flatten them + bufferTime(0), + map(it => [].concat(...it)) ); } @@ -100,27 +91,11 @@ function sliceAndSplice( /** * Creates a new sorted array from a new change. + * Build our own because we allow filtering of action types ('added', 'removed', 'modified') before scanning + * and so we have greater control over change detection (by breaking ===) */ export function combineChange(combined: DocumentChange[], change: DocumentChange): DocumentChange[] { switch (change.type) { - case 'modified': - // OldIndex == -1 is added, since we added modified if fromCache, skip over, don't break - if (change.oldIndex > -1) { - 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) { - const copiedArray = combined.slice(); - copiedArray.splice(change.oldIndex, 1); - copiedArray.splice(change.newIndex, 0, change); - return copiedArray; - } else { - return sliceAndSplice(combined, change.newIndex, 1, change); - } - } - break; - } - // tslint:disable-next-line:no-switch-case-fall-through case 'added': if (combined[change.newIndex] && combined[change.newIndex].doc.ref.isEqual(change.doc.ref)) { // Not sure why the duplicates are getting fired @@ -128,6 +103,20 @@ export function combineChange(combined: DocumentChange[], change: Document 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) { + const copiedArray = combined.slice(); + copiedArray.splice(change.oldIndex, 1); + copiedArray.splice(change.newIndex, 0, change); + return copiedArray; + } else { + return sliceAndSplice(combined, change.newIndex, 1, change); + } + } + break; case 'removed': if (combined[change.oldIndex] && combined[change.oldIndex].doc.ref.isEqual(change.doc.ref)) { return sliceAndSplice(combined, change.oldIndex, 1); From bf0bb00334525aee617dc28415ac77d299a2d48e Mon Sep 17 00:00:00 2001 From: James Daniels Date: Tue, 24 Nov 2020 12:54:51 -0500 Subject: [PATCH 3/3] Nevermind... --- src/firestore/collection/changes.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/firestore/collection/changes.ts b/src/firestore/collection/changes.ts index 8ae7cd357..97a696747 100644 --- a/src/firestore/collection/changes.ts +++ b/src/firestore/collection/changes.ts @@ -1,6 +1,6 @@ import { fromCollectionRef } from '../observable/fromRef'; import { Observable, SchedulerLike } from 'rxjs'; -import { bufferTime, distinctUntilChanged, map, pairwise, scan, startWith } from 'rxjs/operators'; +import { distinctUntilChanged, map, pairwise, scan, startWith } from 'rxjs/operators'; import { DocumentChange, DocumentChangeAction, DocumentChangeType, Query } from '../interfaces'; /** @@ -40,9 +40,6 @@ export function docChanges(query: Query, scheduler?: SchedulerLike): Observab } return actions as DocumentChangeAction[]; }), - // there are some sync changes firing, esp. when taking over from cache, buffer and flatten them - bufferTime(0), - map(it => [].concat(...it)) ); }