Options cleanup for includeMetadataChanges

Options cleanup for includeMetadataChanges

Author: schmidt-sebastian

packages/firestore-types/index.d.ts

@@ -468,13 +468,14 @@ export class WriteBatch {
 }
 
 /**
- * Options for use with `DocumentReference.onSnapshot()` to control the
- * behavior of the snapshot listener.
+ * An options object that can be passed to `DocumentReference.onSnapshot()`,
+ * `Query.onSnapshot()` and `QuerySnapshot.docChanges()` to control which
+ * types of changes to include in the result set.
  */
-export interface DocumentListenOptions {
+export interface SnapshotListenOptions {
   /**
-   * Raise an event even if only metadata of the document changed. Default is
-   * false.
+   * Include a change even if only the metadata of the query or a document
+   * changed. Default is false.
    */
   readonly includeMetadataChanges?: boolean;
 }
@@ -660,7 +661,7 @@ export class DocumentReference {
     complete?: () => void;
   }): () => void;
   onSnapshot(
-    options: DocumentListenOptions,
+    options: SnapshotListenOptions,
     observer: {
       next?: (snapshot: DocumentSnapshot) => void;
       error?: (error: Error) => void;
@@ -673,7 +674,7 @@ export class DocumentReference {
     onCompletion?: () => void
   ): () => void;
   onSnapshot(
-    options: DocumentListenOptions,
+    options: SnapshotListenOptions,
     onNext: (snapshot: DocumentSnapshot) => void,
     onError?: (error: Error) => void,
     onCompletion?: () => void
@@ -843,25 +844,6 @@ export type OrderByDirection = 'desc' | 'asc';
  */
 export type WhereFilterOp = '<' | '<=' | '==' | '>=' | '>';
 
-/**
- * Options for use with `Query.onSnapshot() to control the behavior of the
- * snapshot listener.
- */
-export interface QueryListenOptions {
-  /**
-   * Raise an event even if only metadata changes (i.e. one of the
-   * `QuerySnapshot.metadata` properties). Default is false.
-   */
-  readonly includeQueryMetadataChanges?: boolean;
-
-  /**
-   * Raise an event even if only metadata of a document in the query results
-   * changes (i.e. one of the `DocumentSnapshot.metadata` properties on one of
-   * the documents). Default is false.
-   */
-  readonly includeDocumentMetadataChanges?: boolean;
-}
-
 /**
  * A `Query` refers to a Query which you can read or listen to. You can also
  * construct refined `Query` objects by adding filters and ordering.
@@ -1042,7 +1024,7 @@ export class Query {
     complete?: () => void;
   }): () => void;
   onSnapshot(
-    options: QueryListenOptions,
+    options: SnapshotListenOptions,
     observer: {
       next?: (snapshot: QuerySnapshot) => void;
       error?: (error: Error) => void;
@@ -1055,7 +1037,7 @@ export class Query {
     onCompletion?: () => void
   ): () => void;
   onSnapshot(
-    options: QueryListenOptions,
+    options: SnapshotListenOptions,
     onNext: (snapshot: QuerySnapshot) => void,
     onError?: (error: Error) => void,
     onCompletion?: () => void
@@ -1082,12 +1064,6 @@ export class QuerySnapshot {
    * modifications.
    */
   readonly metadata: SnapshotMetadata;
-  /**
-   * An array of the documents that changed since the last snapshot. If this
-   * is the first snapshot, all documents will be in the list as added
-   * changes.
-   */
-  readonly docChanges: DocumentChange[];
 
   /** An array of all the documents in the QuerySnapshot. */
   readonly docs: QueryDocumentSnapshot[];
@@ -1098,6 +1074,16 @@ export class QuerySnapshot {
   /** True if there are no documents in the QuerySnapshot. */
   readonly empty: boolean;
 
+  /**
+   * Returns an array of the documents changes since the last snapshot. If this
+   * is the first snapshot, all documents will be in the list as added changes.
+   *
+   * @param options `SnapshotListenOptions` that control whether metadata-only
+   * changes (i.e. only `DocumentSnapshot.metadata` changed) should trigger
+   * snapshot events.
+   */
+  docChanges(options?: SnapshotListenOptions): DocumentChange[];
+
   /**
    * Enumerates all of the documents in the QuerySnapshot.
    *

packages/firestore/CHANGELOG.md

@@ -1,4 +1,11 @@
 # Unreleased
+- [changed] Merged the `includeQueryMetadataChanges` and 
+  `includeDocumentMetadataChanges` options passed to `Query.onSnapshot()` into 
+  a single `includeMetadataChanges` option.
+- [changed] `QuerySnapshot.docChanges()` is now a method that optionally takes
+  an `includeMetadataChanges` option. By default, even when listening to a query
+  with `{ includeMetadataChanges:true }`, metadata-only document changes are 
+  suppressed in `docChanges()`. 
 - [fixed] Fixed a regression in the Firebase JS release 4.11.0 that could
   cause get() requests made while offline to be delayed by up to 10
   seconds (rather than returning from cache immediately).

packages/firestore/src/api/database.ts

@@ -877,7 +877,7 @@ export class DocumentReference implements firestore.DocumentReference {
     observer: PartialObserver<firestore.DocumentSnapshot>
   ): Unsubscribe;
   onSnapshot(
-    options: firestore.DocumentListenOptions,
+    options: firestore.SnapshotListenOptions,
     observer: PartialObserver<firestore.DocumentSnapshot>
   ): Unsubscribe;
   onSnapshot(
@@ -886,7 +886,7 @@ export class DocumentReference implements firestore.DocumentReference {
     onCompletion?: CompleteFn
   ): Unsubscribe;
   onSnapshot(
-    options: firestore.DocumentListenOptions,
+    options: firestore.SnapshotListenOptions,
     onNext: NextFn<firestore.DocumentSnapshot>,
     onError?: ErrorFn,
     onCompletion?: CompleteFn
@@ -899,7 +899,7 @@ export class DocumentReference implements firestore.DocumentReference {
       1,
       4
     );
-    let options: firestore.DocumentListenOptions = {
+    let options: firestore.SnapshotListenOptions = {
       includeMetadataChanges: false
     };
     let observer: PartialObserver<firestore.DocumentSnapshot>;
@@ -908,7 +908,7 @@ export class DocumentReference implements firestore.DocumentReference {
       typeof args[currArg] === 'object' &&
       !isPartialObserver(args[currArg])
     ) {
-      options = args[currArg] as firestore.DocumentListenOptions;
+      options = args[currArg] as firestore.SnapshotListenOptions;
       validateOptionNames('DocumentReference.onSnapshot', options, [
         'includeMetadataChanges'
       ]);
@@ -922,8 +922,7 @@ export class DocumentReference implements firestore.DocumentReference {
     }
 
     const internalOptions = {
-      includeDocumentMetadataChanges: options.includeMetadataChanges,
-      includeQueryMetadataChanges: options.includeMetadataChanges
+      includeMetadataChanges: options.includeMetadataChanges
     };
 
     if (isPartialObserver(args[currArg])) {
@@ -1041,8 +1040,7 @@ export class DocumentReference implements firestore.DocumentReference {
   ): void {
     const unlisten = this.onSnapshotInternal(
       {
-        includeQueryMetadataChanges: true,
-        includeDocumentMetadataChanges: true,
+        includeMetadataChanges: true,
         waitForSyncWhenOnline: true
       },
       {
@@ -1566,7 +1564,7 @@ export class Query implements firestore.Query {
 
   onSnapshot(observer: PartialObserver<firestore.QuerySnapshot>): Unsubscribe;
   onSnapshot(
-    options: firestore.QueryListenOptions,
+    options: firestore.SnapshotListenOptions,
     observer: PartialObserver<firestore.QuerySnapshot>
   ): Unsubscribe;
   onSnapshot(
@@ -1575,37 +1573,30 @@ export class Query implements firestore.Query {
     onCompletion?: CompleteFn
   ): Unsubscribe;
   onSnapshot(
-    options: firestore.QueryListenOptions,
+    options: firestore.SnapshotListenOptions,
     onNext: NextFn<firestore.QuerySnapshot>,
     onError?: ErrorFn,
     onCompletion?: CompleteFn
   ): Unsubscribe;
 
   onSnapshot(...args: AnyJs[]): Unsubscribe {
     validateBetweenNumberOfArgs('Query.onSnapshot', arguments, 1, 4);
-    let options: firestore.QueryListenOptions = {};
+    let options: firestore.SnapshotListenOptions = {};
     let observer: PartialObserver<firestore.QuerySnapshot>;
     let currArg = 0;
     if (
       typeof args[currArg] === 'object' &&
       !isPartialObserver(args[currArg])
     ) {
-      options = args[currArg] as firestore.QueryListenOptions;
+      options = args[currArg] as firestore.SnapshotListenOptions;
       validateOptionNames('Query.onSnapshot', options, [
-        'includeQueryMetadataChanges',
-        'includeDocumentMetadataChanges'
+        'includeMetadataChanges'
       ]);
       validateNamedOptionalType(
         'Query.onSnapshot',
         'boolean',
-        'includeDocumentMetadataChanges',
-        options.includeDocumentMetadataChanges
-      );
-      validateNamedOptionalType(
-        'Query.onSnapshot',
-        'boolean',
-        'includeQueryMetadataChanges',
-        options.includeQueryMetadataChanges
+        'includeMetadataChanges',
+        options.includeMetadataChanges
       );
       currArg++;
     }
@@ -1692,8 +1683,7 @@ export class Query implements firestore.Query {
   ): void {
     const unlisten = this.onSnapshotInternal(
       {
-        includeDocumentMetadataChanges: false,
-        includeQueryMetadataChanges: true,
+        includeMetadataChanges: true,
         waitForSyncWhenOnline: true
       },
       {
@@ -1774,6 +1764,7 @@ export class Query implements firestore.Query {
 
 export class QuerySnapshot implements firestore.QuerySnapshot {
   private _cachedChanges: firestore.DocumentChange[] | null = null;
+  private _cachedChangesIncludeMetadataChanges: boolean | null = null;
 
   readonly metadata: firestore.SnapshotMetadata;
 
@@ -1817,13 +1808,44 @@ export class QuerySnapshot implements firestore.QuerySnapshot {
     return new Query(this._originalQuery, this._firestore);
   }
 
-  get docChanges(): firestore.DocumentChange[] {
-    if (!this._cachedChanges) {
+  docChanges(
+    options?: firestore.SnapshotListenOptions
+  ): firestore.DocumentChange[] {
+    validateOptionNames('QuerySnapshot.docChanges', options, [
+      'includeMetadataChanges'
+    ]);
+
+    if (options) {
+      validateNamedOptionalType(
+        'QuerySnapshot.docChanges',
+        'boolean',
+        'includeMetadataChanges',
+        options.includeMetadataChanges
+      );
+    }
+
+    const includeMetadataChanges = options && options.includeMetadataChanges;
+
+    if (includeMetadataChanges && this._snapshot.excludesMetadataChanges) {
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
+        'To include metadata changes with your document changes, you must ' +
+          'also pass { includeMetadataChanges:true } to onSnapshot().'
+      );
+    }
+
+    if (
+      !this._cachedChanges ||
+      this._cachedChangesIncludeMetadataChanges !== includeMetadataChanges
+    ) {
       this._cachedChanges = changesFromSnapshot(
         this._firestore,
+        includeMetadataChanges,
         this._snapshot
       );
+      this._cachedChangesIncludeMetadataChanges = includeMetadataChanges;
     }
+
     return this._cachedChanges;
   }
 
@@ -1850,6 +1872,30 @@ export class QuerySnapshot implements firestore.QuerySnapshot {
   }
 }
 
+// TODO(2018/11/01): As of 2018/04/17 we're changing docChanges from an array
+// into a method. Because this is a runtime breaking change and somewhat subtle
+// (both Array and Function have a .length, etc.), we'll replace the .length and
+// @@iterator properties to throw a custom error message. In ~6 months we can
+// delete the custom error as most folks will have hopefully migrated.
+function throwDocChangesMethodError(): never {
+  throw new FirestoreError(
+    Code.INVALID_ARGUMENT,
+    'QuerySnapshot.docChanges has been changed from a property into a ' +
+      'method, so usages like "querySnapshot.docChanges" should become ' +
+      '"querySnapshot.docChanges()"'
+  );
+}
+
+Object.defineProperty(QuerySnapshot.prototype.docChanges, 'length', {
+  get: () => throwDocChangesMethodError()
+});
+
+if (typeof Symbol !== 'undefined') {
+  Object.defineProperty(QuerySnapshot.prototype.docChanges, Symbol.iterator, {
+    get: () => throwDocChangesMethodError()
+  });
+}
+
 export class CollectionReference extends Query
   implements firestore.CollectionReference {
   constructor(path: ResourcePath, firestore: Firestore) {
@@ -1968,6 +2014,7 @@ function validateReference(
  */
 export function changesFromSnapshot(
   firestore: Firestore,
+  includeMetadataChanges: boolean,
   snapshot: ViewSnapshot
 ): firestore.DocumentChange[] {
   if (snapshot.oldDocs.isEmpty()) {
@@ -2002,26 +2049,30 @@ export function changesFromSnapshot(
     // A DocumentSet that is updated incrementally as changes are applied to use
     // to lookup the index of a document.
     let indexTracker = snapshot.oldDocs;
-    return snapshot.docChanges.map(change => {
-      const doc = new QueryDocumentSnapshot(
-        firestore,
-        change.doc.key,
-        change.doc,
-        snapshot.fromCache
-      );
-      let oldIndex = -1;
-      let newIndex = -1;
-      if (change.type !== ChangeType.Added) {
-        oldIndex = indexTracker.indexOf(change.doc.key);
-        assert(oldIndex >= 0, 'Index for document not found');
-        indexTracker = indexTracker.delete(change.doc.key);
-      }
-      if (change.type !== ChangeType.Removed) {
-        indexTracker = indexTracker.add(change.doc);
-        newIndex = indexTracker.indexOf(change.doc.key);
-      }
-      return { type: resultChangeType(change.type), doc, oldIndex, newIndex };
-    });
+    return snapshot.docChanges
+      .filter(
+        change => includeMetadataChanges || change.type !== ChangeType.Metadata
+      )
+      .map(change => {
+        const doc = new QueryDocumentSnapshot(
+          firestore,
+          change.doc.key,
+          change.doc,
+          snapshot.fromCache
+        );
+        let oldIndex = -1;
+        let newIndex = -1;
+        if (change.type !== ChangeType.Added) {
+          oldIndex = indexTracker.indexOf(change.doc.key);
+          assert(oldIndex >= 0, 'Index for document not found');
+          indexTracker = indexTracker.delete(change.doc.key);
+        }
+        if (change.type !== ChangeType.Removed) {
+          indexTracker = indexTracker.add(change.doc);
+          newIndex = indexTracker.indexOf(change.doc.key);
+        }
+        return { type: resultChangeType(change.type), doc, oldIndex, newIndex };
+      });
   }
 }