wip removed unnecessary converters in classes

googleapis · Dec 20, 2019 · 3aa76b2 · 3aa76b2
1 parent 4eacbce
commit 3aa76b2
Show file tree

Hide file tree

Showing 12 changed files with 189 additions and 81 deletions.
diff --git a/dev/src/document-change.ts b/dev/src/document-change.ts
@@ -14,9 +14,8 @@
  * limitations under the License.
  */
 
-import {FirestoreDataConverter} from '@google-cloud/firestore';
 import {QueryDocumentSnapshot} from './document';
-import {DocumentData} from './types';
+import {DocumentData, FirestoreDataConverter} from './types';
 import {defaultConverter} from './watch';
 
 export type DocumentChangeType = 'added' | 'removed' | 'modified';
@@ -32,7 +31,6 @@ export class DocumentChange<T = DocumentData> {
   private readonly _document: QueryDocumentSnapshot<T>;
   private readonly _oldIndex: number;
   private readonly _newIndex: number;
-  private readonly _converter?: FirestoreDataConverter<T>;
 
   /**
    * @hideconstructor
@@ -48,15 +46,12 @@ export class DocumentChange<T = DocumentData> {
     type: DocumentChangeType,
     document: QueryDocumentSnapshot<T>,
     oldIndex: number,
-    newIndex: number,
-    converter?: FirestoreDataConverter<T>
+    newIndex: number
   ) {
     this._type = type;
     this._document = document;
     this._oldIndex = oldIndex;
     this._newIndex = newIndex;
-    this._converter =
-      converter || (defaultConverter as FirestoreDataConverter<T>);
   }
 
   /**
@@ -186,8 +181,7 @@ export class DocumentChange<T = DocumentData> {
       this._type === other._type &&
       this._oldIndex === other._oldIndex &&
       this._newIndex === other._newIndex &&
-      this._document.isEqual(other._document) &&
-      this._converter === other._converter
+      this._document.isEqual(other._document)
     );
   }
 }
diff --git a/dev/src/document.ts b/dev/src/document.ts
@@ -28,8 +28,6 @@ import {ApiMapValue, DocumentData, UpdateMap} from './types';
 import {isEmpty, isObject} from './util';
 
 import api = google.firestore.v1;
-import {FirestoreDataConverter} from '@google-cloud/firestore';
-import {defaultConverter} from './watch';
 
 /**
  * Returns a builder for DocumentSnapshot and QueryDocumentSnapshot instances.
@@ -39,7 +37,7 @@ import {defaultConverter} from './watch';
  */
 export class DocumentSnapshotBuilder<T = DocumentData> {
   /** The reference to the document. */
-  ref?: DocumentReference<T>;
+  ref: DocumentReference<T>;
 
   /** The fields of the Firestore `Document` Protobuf backing this document. */
   fieldsProto?: ApiMapValue;
@@ -53,11 +51,12 @@ export class DocumentSnapshotBuilder<T = DocumentData> {
   /** The time when this document was last updated. */
   updateTime?: Timestamp;
 
-  private readonly _converter: FirestoreDataConverter<T>;
-
-  constructor(converter?: FirestoreDataConverter<T>) {
-    this._converter =
-      converter || (defaultConverter as FirestoreDataConverter<T>);
+  /**
+   * We include the DocumentReference in the constructor in order to allow the
+   * DocumentSnapshotBuilder to be typed with <T> when it is constructed.
+   */
+  constructor(ref: DocumentReference<T>) {
+    this.ref = ref;
   }
 
   /**
@@ -82,16 +81,14 @@ export class DocumentSnapshotBuilder<T = DocumentData> {
           this.fieldsProto!,
           this.readTime!,
           this.createTime!,
-          this.updateTime!,
-          this.ref!._converter
+          this.updateTime!
         )
       : new DocumentSnapshot(
           this.ref!,
           undefined,
           this.readTime!,
           undefined,
-          undefined,
-          this.ref!._converter
+          undefined
         );
   }
 }
@@ -117,7 +114,6 @@ export class DocumentSnapshot<T = DocumentData> {
   private _readTime: Timestamp | undefined;
   private _createTime: Timestamp | undefined;
   private _updateTime: Timestamp | undefined;
-  private readonly _converter: FirestoreDataConverter<T>;
 
   /**
    * @hideconstructor
@@ -137,17 +133,14 @@ export class DocumentSnapshot<T = DocumentData> {
     fieldsProto?: ApiMapValue,
     readTime?: Timestamp,
     createTime?: Timestamp,
-    updateTime?: Timestamp,
-    converter?: FirestoreDataConverter<T>
+    updateTime?: Timestamp
   ) {
     this._ref = ref;
     this._fieldsProto = fieldsProto;
     this._serializer = ref.firestore._serializer!;
     this._readTime = readTime;
     this._createTime = createTime;
     this._updateTime = updateTime;
-    this._converter =
-      converter || (defaultConverter as FirestoreDataConverter<T>);
   }
 
   /**
@@ -408,7 +401,7 @@ export class DocumentSnapshot<T = DocumentData> {
     for (const prop of Object.keys(fields)) {
       obj[prop] = this._serializer.decodeValue(fields[prop]);
     }
-    return this._converter.fromFirestore(obj);
+    return this.ref._converter.fromFirestore(obj);
   }
 
   /**
@@ -515,8 +508,7 @@ export class DocumentSnapshot<T = DocumentData> {
       this === other ||
       (other instanceof DocumentSnapshot &&
         this._ref.isEqual(other._ref) &&
-        deepEqual(this._fieldsProto, other._fieldsProto, {strict: true}) &&
-        this._converter === other._converter)
+        deepEqual(this._fieldsProto, other._fieldsProto, {strict: true}))
     );
   }
 }
@@ -554,10 +546,9 @@ export class QueryDocumentSnapshot<T = DocumentData> extends DocumentSnapshot<
     fieldsProto: ApiMapValue,
     readTime: Timestamp,
     createTime: Timestamp,
-    updateTime: Timestamp,
-    converter?: FirestoreDataConverter<T>
+    updateTime: Timestamp
   ) {
-    super(ref, fieldsProto, readTime, createTime, updateTime, converter);
+    super(ref, fieldsProto, readTime, createTime, updateTime);
   }
 
   /**

diff --git a/dev/src/index.ts b/dev/src/index.ts
@@ -42,6 +42,7 @@ import {Timestamp} from './timestamp';
 import {parseGetAllArguments, Transaction} from './transaction';
 import {
   ApiMapValue,
+  DocumentData,
   GapicClient,
   GrpcError,
   ReadOptions,
@@ -60,7 +61,6 @@ import {
 import {WriteBatch} from './write-batch';
 
 import api = google.firestore.v1;
-import {DocumentData} from '@google-cloud/firestore';
 
 export {
   CollectionReference,
@@ -661,20 +661,22 @@ export class Firestore {
       );
     }
 
-    const document = new DocumentSnapshotBuilder<T>();
-
+    let ref;
+    let document;
     if (typeof documentOrName === 'string') {
-      document.ref = new DocumentReference<T>(
+      ref = new DocumentReference<T>(
         this,
         QualifiedResourcePath.fromSlashSeparatedString(documentOrName)
       );
+      document = new DocumentSnapshotBuilder(ref);
     } else {
-      document.ref = new DocumentReference<T>(
+      ref = new DocumentReference<T>(
         this,
         QualifiedResourcePath.fromSlashSeparatedString(
           documentOrName.name as string
         )
       );
+      document = new DocumentSnapshotBuilder(ref);
       document.fieldsProto = documentOrName.fields
         ? convertFields(documentOrName.fields as ApiMapValue)
         : {};

diff --git a/dev/src/reference.ts b/dev/src/reference.ts
@@ -41,6 +41,7 @@ import {Serializable, Serializer, validateUserInput} from './serializer';
 import {Timestamp} from './timestamp';
 import {
   DocumentData,
+  FirestoreDataConverter,
   OrderByDirection,
   Precondition,
   SetOptions,
@@ -59,7 +60,6 @@ import {defaultConverter, DocumentWatch, QueryWatch} from './watch';
 import {validateDocumentData, WriteBatch, WriteResult} from './write-batch';
 
 import api = proto.google.firestore.v1;
-import {FirestoreDataConverter} from '@google-cloud/firestore';
 
 /**
  * The direction of a `Query.orderBy()` clause is specified as 'desc' or 'asc'
@@ -497,12 +497,12 @@ export class DocumentReference<T = DocumentData> implements Serializable {
       }
 
       // The document is missing.
-      const document = new DocumentSnapshotBuilder<T>();
-      document.ref = new DocumentReference(
+      const ref = new DocumentReference(
         this._firestore,
         this._path,
         this._converter
       );
+      const document = new DocumentSnapshotBuilder<T>(ref);
       document.readTime = readTime;
       onNext(document.build());
     }, onError || console.error);
@@ -675,7 +675,6 @@ export class QuerySnapshot<T = DocumentData> {
   private _materializedChanges: Array<DocumentChange<T>> | null = null;
   private _docs: (() => Array<QueryDocumentSnapshot<T>>) | null = null;
   private _changes: (() => Array<DocumentChange<T>>) | null = null;
-  private readonly _converter: FirestoreDataConverter<T>;
 
   /**
    * @hideconstructor
@@ -695,13 +694,10 @@ export class QuerySnapshot<T = DocumentData> {
     private readonly _readTime: Timestamp,
     private readonly _size: number,
     docs: () => Array<QueryDocumentSnapshot<T>>,
-    changes: () => Array<DocumentChange<T>>,
-    converter?: FirestoreDataConverter<T>
+    changes: () => Array<DocumentChange<T>>
   ) {
     this._docs = docs;
     this._changes = changes;
-    this._converter =
-      converter || (defaultConverter as FirestoreDataConverter<T>);
   }
 
   /**
@@ -901,10 +897,6 @@ export class QuerySnapshot<T = DocumentData> {
       );
     }
 
-    if (this._converter !== other._converter) {
-      return false;
-    }
-
     // Otherwise, we compare the changes first as we expect there to be fewer.
     return (
       isArrayEqual(this.docChanges(), other.docChanges()) &&
@@ -1040,7 +1032,7 @@ export class QueryOptions {
  */
 export class Query<T = DocumentData> {
   private readonly _serializer: Serializer;
-  private readonly _converter: FirestoreDataConverter<T>;
+  readonly _converter: FirestoreDataConverter<T>;
 
   /**
    * @hideconstructor
@@ -1724,13 +1716,10 @@ export class Query<T = DocumentData> {
               () => {
                 const changes: Array<DocumentChange<T>> = [];
                 for (let i = 0; i < docs.length; ++i) {
-                  changes.push(
-                    new DocumentChange('added', docs[i], -1, i, this._converter)
-                  );
+                  changes.push(new DocumentChange('added', docs[i], -1, i));
                 }
                 return changes;
-              },
-              this._converter
+              }
             )
           );
         });
@@ -2178,7 +2167,7 @@ export class CollectionReference<T = DocumentData> extends Query<T> {
       );
     }
 
-    return new DocumentReference(this.firestore, path);
+    return new DocumentReference(this.firestore, path, this._converter);
   }
 
   /**
@@ -2258,13 +2247,6 @@ export function validateQueryOrder(
   return op as OrderByDirection | undefined;
 }
 
-export function defaultDataConverter(): FirestoreDataConverter<DocumentData> {
-  return {
-    toFirestore: data => data,
-    fromFirestore: data => data as DocumentData,
-  };
-}
-
 /**
  * Validates the input string as a field comparison operator.
  *

diff --git a/dev/src/types.ts b/dev/src/types.ts
@@ -40,6 +40,61 @@ export class GrpcError extends Error {
   code?: number;
 }
 
+/**
+ * Converter used by `withConverter()` to transform user objects of type T
+ * into Firestore data.
+ *
+ * Using the converter allows you to specify generic type arguments when
+ * storing and retrieving objects from Firestore.
+ *
+ * @example
+ * ```typescript
+ * class Post {
+ *   constructor(readonly title: string, readonly author: string) {}
+ *
+ *   toString(): string {
+ *     return this.title + ', by ' + this.author;
+ *   }
+ * }
+ *
+ * const postConverter = {
+ *   toFirestore(post: Post): FirebaseFirestore.DocumentData {
+ *     return {title: post.title, author: post.author};
+ *   },
+ *   fromFirestore(
+ *     data: FirebaseFirestore.DocumentData
+ *   ): Post {
+ *     return new Post(data.title, data.author);
+ *   }
+ * };
+ *
+ * const postSnap = await Firestore()
+ *   .collection('posts')
+ *   .withConverter(postConverter)
+ *   .doc().get();
+ * const post = postSnap.data();
+ * if (post !== undefined) {
+ *   post.title; // string
+ *   post.toString(); // Should be defined
+ *   post.someNonExistentProperty; // TS error
+ * }
+ * ```
+ */
+export interface FirestoreDataConverter<T> {
+  /**
+   * Called by the Firestore SDK to convert a custom model object of type T
+   * into a plain Javascript object (suitable for writing directly to the
+   * Firestore database).
+   */
+  toFirestore(modelObject: T): DocumentData;
+
+  /**
+   * Called by the Firestore SDK to convert Firestore data into an object of
+   * type T.
+   */
+  fromFirestore(data: DocumentData): T;
+}
+
 /**
  * Settings used to directly configure a `Firestore` instance.
  */