Making DocumentSnaphot.data() nullable (#364)

schmidt-sebastian · web-flow · commit 39d1bffb6563 · 2017-12-13T17:07:58.000+08:00
* Making DocumentSnapshot nullable
diff --git a/packages/firestore-types/index.d.ts b/packages/firestore-types/index.d.ts
@@ -539,9 +539,13 @@ export interface SnapshotMetadata {
  * A `DocumentSnapshot` contains data read from a document in your Firestore
  * database. The data can be extracted with `.data()` or `.get(<field>)` to
  * get a specific field.
+ *
+ * For a `DocumentSnapshot` that points to a non-existing document, any data
+ * access will return 'undefined'. You can use the `exists` property to
+ * explicitly verify a document's existence.
  */
 export class DocumentSnapshot {
-  private constructor();
+  protected constructor();
 
   /** True if the document exists. */
   readonly exists: boolean;
@@ -558,14 +562,17 @@ export class DocumentSnapshot {
   readonly metadata: SnapshotMetadata;
 
   /**
-   * Retrieves all fields in the document as an Object.
+   * Retrieves all fields in the document as an Object. Returns 'undefined' if
+   * the document doesn't exist.
    *
-   * @return An Object containing all fields in the document.
+   * @return An Object containing all fields in the document or 'undefined' if
+   * the document doesn't exist.
    */
-  data(): DocumentData;
+  data(): DocumentData | undefined;
 
   /**
-   * Retrieves the field specified by `fieldPath`.
+   * Retrieves the field specified by `fieldPath`. Returns 'undefined' if the
+   * document or field doesn't exist.
    *
    * @param fieldPath The path (e.g. 'foo' or 'foo.bar') to a specific field.
    * @return The data at the specified field location or undefined if no such
@@ -574,6 +581,29 @@ export class DocumentSnapshot {
   get(fieldPath: string | FieldPath): any;
 }
 
+/**
+ * A `QueryDocumentSnapshot` contains data read from a document in your
+ * Firestore database as part of a query. The document is guaranteed to exist
+ * and its data can be extracted with `.data()` or `.get(<field>)` to get a
+ * specific field.
+ *
+ * A `QueryDocumentSnapshot` offers the same API surface as a
+ * `DocumentSnapshot`. Since query results contain only existing documents, the
+ * `exists` property will always be true and `data()` will never return
+ * 'undefined'.
+ */
+export class QueryDocumentSnapshot extends DocumentSnapshot {
+  private constructor();
+
+  /**
+   * Retrieves all fields in the document as an Object.
+   *
+   * @override
+   * @return An Object containing all fields in the document.
+   */
+  data(): DocumentData;
+}
+
 /**
  * The direction of a `Query.orderBy()` clause is specified as 'desc' or 'asc'
  * (descending or ascending).
@@ -827,7 +857,7 @@ export class QuerySnapshot {
   readonly docChanges: DocumentChange[];
 
   /** An array of all the documents in the QuerySnapshot. */
-  readonly docs: DocumentSnapshot[];
+  readonly docs: QueryDocumentSnapshot[];
 
   /** The number of documents in the QuerySnapshot. */
   readonly size: number;
@@ -838,11 +868,14 @@ export class QuerySnapshot {
   /**
    * Enumerates all of the documents in the QuerySnapshot.
    *
-   * @param callback A callback to be called with a `DocumentSnapshot` for
+   * @param callback A callback to be called with a `QueryDocumentSnapshot` for
    * each document in the snapshot.
    * @param thisArg The `this` binding for the callback.
    */
-  forEach(callback: (result: DocumentSnapshot) => void, thisArg?: any): void;
+  forEach(
+    callback: (result: QueryDocumentSnapshot) => void,
+    thisArg?: any
+  ): void;
 }
 
 /**
@@ -859,7 +892,7 @@ export interface DocumentChange {
   readonly type: DocumentChangeType;
 
   /** The document affected by this change. */
-  readonly doc: DocumentSnapshot;
+  readonly doc: QueryDocumentSnapshot;
 
   /**
    * The index of the changed document in the result set immediately prior to
diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts
@@ -991,31 +991,24 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
     private _fromCache: boolean
   ) {}
 
-  data(): firestore.DocumentData {
+  data(): firestore.DocumentData | undefined {
     validateExactNumberOfArgs('DocumentSnapshot.data', arguments, 0);
-    if (!this._document) {
-      throw new FirestoreError(
-        Code.NOT_FOUND,
-        "This document doesn't exist. Check doc.exists to make sure " +
-          'the document exists before calling doc.data().'
-      );
-    }
-    return this.convertObject(this._document.data);
+    return !this._document
+      ? undefined
+      : this.convertObject(this._document.data);
   }
 
   get(fieldPath: string | ExternalFieldPath): AnyJs {
     validateExactNumberOfArgs('DocumentSnapshot.get', arguments, 1);
-    if (!this._document) {
-      throw new FirestoreError(
-        Code.NOT_FOUND,
-        "This document doesn't exist. Check doc.exists to make sure " +
-          'the document exists before calling doc.get().'
+    if (this._document) {
+      const value = this._document.data.field(
+        fieldPathFromArgument('DocumentSnapshot.get', fieldPath)
       );
+      if (value !== undefined) {
+        return this.convertValue(value);
+      }
     }
-    const value = this._document.data.field(
-      fieldPathFromArgument('DocumentSnapshot.get', fieldPath)
-    );
-    return value === undefined ? undefined : this.convertValue(value);
+    return undefined;
   }
 
   get id(): string {
@@ -1080,6 +1073,27 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
   }
 }
 
+export class QueryDocumentSnapshot extends DocumentSnapshot
+  implements firestore.QueryDocumentSnapshot {
+  constructor(
+    firestore: Firestore,
+    key: DocumentKey,
+    document: Document,
+    fromCache: boolean
+  ) {
+    super(firestore, key, document, fromCache);
+  }
+
+  data(): firestore.DocumentData {
+    const data = super.data();
+    assert(
+      typeof data === 'object',
+      'Document in a QueryDocumentSnapshot should exist'
+    );
+    return data as firestore.DocumentData;
+  }
+}
+
 export class Query implements firestore.Query {
   constructor(public _query: InternalQuery, readonly firestore: Firestore) {}
 
@@ -1579,8 +1593,8 @@ export class QuerySnapshot implements firestore.QuerySnapshot {
     };
   }
 
-  get docs(): firestore.DocumentSnapshot[] {
-    const result: firestore.DocumentSnapshot[] = [];
+  get docs(): firestore.QueryDocumentSnapshot[] {
+    const result: firestore.QueryDocumentSnapshot[] = [];
     this.forEach(doc => result.push(doc));
     return result;
   }
@@ -1594,7 +1608,7 @@ export class QuerySnapshot implements firestore.QuerySnapshot {
   }
 
   forEach(
-    callback: (result: firestore.DocumentSnapshot) => void,
+    callback: (result: firestore.QueryDocumentSnapshot) => void,
     thisArg?: AnyJs
   ): void {
     validateBetweenNumberOfArgs('QuerySnapshot.forEach', arguments, 1, 2);
@@ -1618,8 +1632,8 @@ export class QuerySnapshot implements firestore.QuerySnapshot {
     return this._cachedChanges;
   }
 
-  private convertToDocumentImpl(doc: Document): DocumentSnapshot {
-    return new DocumentSnapshot(
+  private convertToDocumentImpl(doc: Document): QueryDocumentSnapshot {
+    return new QueryDocumentSnapshot(
       this._firestore,
       doc.key,
       doc,
@@ -1735,7 +1749,7 @@ export function changesFromSnapshot(
     let lastDoc: Document;
     let index = 0;
     return snapshot.docChanges.map(change => {
-      const doc = new DocumentSnapshot(
+      const doc = new QueryDocumentSnapshot(
         firestore,
         change.doc.key,
         change.doc,
@@ -1762,7 +1776,7 @@ export function changesFromSnapshot(
     // to lookup the index of a document.
     let indexTracker = snapshot.oldDocs;
     return snapshot.docChanges.map(change => {
-      const doc = new DocumentSnapshot(
+      const doc = new QueryDocumentSnapshot(
         firestore,
         change.doc.key,
         change.doc,
@@ -1821,6 +1835,9 @@ export const PublicDocumentReference = makeConstructorPrivate(
   'Use firebase.firestore().doc() instead.'
 );
 export const PublicDocumentSnapshot = makeConstructorPrivate(DocumentSnapshot);
+export const PublicQueryDocumentSnapshot = makeConstructorPrivate(
+  QueryDocumentSnapshot
+);
 export const PublicQuery = makeConstructorPrivate(Query);
 export const PublicQuerySnapshot = makeConstructorPrivate(QuerySnapshot);
 export const PublicCollectionReference = makeConstructorPrivate(
diff --git a/packages/firestore/src/platform/config.ts b/packages/firestore/src/platform/config.ts
@@ -25,6 +25,7 @@ import {
   PublicDocumentSnapshot,
   PublicFirestore,
   PublicQuery,
+  PublicQueryDocumentSnapshot,
   PublicQuerySnapshot,
   PublicTransaction,
   PublicWriteBatch
@@ -43,6 +44,7 @@ const firestoreNamespace = {
   DocumentReference: PublicDocumentReference,
   DocumentSnapshot: PublicDocumentSnapshot,
   Query: PublicQuery,
+  QueryDocumentSnapshot: PublicQueryDocumentSnapshot,
   QuerySnapshot: PublicQuerySnapshot,
   CollectionReference: PublicCollectionReference,
   FieldPath: FieldPath,
diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts
@@ -94,6 +94,16 @@ apiDescribe('Database', persistence => {
     });
   });
 
+  it('can retrieve document that does not exist', () => {
+    return withTestDoc(persistence, doc => {
+      return doc.get().then(snapshot => {
+        expect(snapshot.exists).to.equal(false);
+        expect(snapshot.data()).to.equal(undefined);
+        expect(snapshot.get('foo')).to.equal(undefined);
+      });
+    });
+  });
+
   it('can merge data with an existing document using set', () => {
     return withTestDoc(persistence, doc => {
       const initialData = {
