docs: FirestoreDataConverter doc and test improvements (#1988)

dev/test/types.ts

@@ -12,7 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-import {expect} from 'chai';
 import {
   QueryDocumentSnapshot,
   DocumentReference,
@@ -22,6 +21,7 @@ import {
   FirestoreDataConverter,
   SetOptions,
 } from '@google-cloud/firestore';
+
 describe('FirestoreTypeConverter', () => {
   it('converter has the minimal typing information', () => {
     interface MyModelType {
@@ -39,17 +39,13 @@ describe('FirestoreTypeConverter', () => {
         };
       },
     };
-    // The intent of the function below is to test TypeScript compile and not execute.
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    async function _(docRef: DocumentReference): Promise<void> {
+    neverCall<Promise<MyModelType>>(async docRef => {
       const newDocRef = docRef.withConverter(converter);
       await newDocRef.set({stringProperty: 'foo', numberProperty: 42});
       await newDocRef.update({a: 'newFoo', b: 43});
       const snapshot = await newDocRef.get();
-      const data: MyModelType = snapshot.data()!;
-      expect(data.stringProperty).to.equal('newFoo');
-      expect(data.numberProperty).to.equal(43);
-    }
+      return snapshot.data()!;
+    });
   });
 
   it('converter has the minimal typing information plus return types', () => {
@@ -68,17 +64,13 @@ describe('FirestoreTypeConverter', () => {
         };
       },
     };
-    // The intent of the function below is to test TypeScript compile and not execute.
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    async function _(docRef: DocumentReference): Promise<void> {
+    neverCall<Promise<MyModelType>>(async docRef => {
       const newDocRef = docRef.withConverter(converter);
       await newDocRef.set({stringProperty: 'foo', numberProperty: 42});
       await newDocRef.update({a: 'newFoo', b: 43});
       const snapshot = await newDocRef.get();
-      const data: MyModelType = snapshot.data()!;
-      expect(data.stringProperty).to.equal('newFoo');
-      expect(data.numberProperty).to.equal(43);
-    }
+      return snapshot.data()!;
+    });
   });
 
   it("has the additional 'merge' version of toFirestore()", () => {
@@ -113,17 +105,13 @@ describe('FirestoreTypeConverter', () => {
         };
       },
     };
-    // The intent of the function below is to test TypeScript compile and not execute.
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    async function _(docRef: DocumentReference): Promise<void> {
+    neverCall<Promise<MyModelType>>(async docRef => {
       const newDocRef = docRef.withConverter(converter);
       await newDocRef.set({stringProperty: 'foo', numberProperty: 42});
       await newDocRef.update({a: 'newFoo', b: 43});
       const snapshot = await newDocRef.get();
-      const data: MyModelType = snapshot.data()!;
-      expect(data.stringProperty).to.equal('newFoo');
-      expect(data.numberProperty).to.equal(43);
-    }
+      return snapshot.data()!;
+    });
   });
 
   it('converter is explicitly typed as FirestoreDataConverter<T>', () => {
@@ -142,17 +130,13 @@ describe('FirestoreTypeConverter', () => {
         };
       },
     };
-    // The intent of the function below is to test TypeScript compile and not execute.
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    async function _(docRef: DocumentReference): Promise<void> {
+    neverCall<Promise<MyModelType>>(async docRef => {
       const newDocRef = docRef.withConverter(converter);
       await newDocRef.set({stringProperty: 'foo', numberProperty: 42});
       await newDocRef.update({a: 'newFoo', b: 43});
       const snapshot = await newDocRef.get();
-      const data: MyModelType = snapshot.data()!;
-      expect(data.stringProperty).to.equal('newFoo');
-      expect(data.numberProperty).to.equal(43);
-    }
+      return snapshot.data()!;
+    });
   });
 
   it('converter is explicitly typed as FirestoreDataConverter<T, U>', () => {
@@ -175,16 +159,92 @@ describe('FirestoreTypeConverter', () => {
         };
       },
     };
-    // The intent of the function below is to test TypeScript compile and not execute.
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    async function _(docRef: DocumentReference): Promise<void> {
+    neverCall<Promise<MyModelType>>(async docRef => {
       const newDocRef = docRef.withConverter(converter);
       await newDocRef.set({stringProperty: 'foo', numberProperty: 42});
       await newDocRef.update({a: 'newFoo', b: 43});
       const snapshot = await newDocRef.get();
-      const data: MyModelType = snapshot.data()!;
-      expect(data.stringProperty).to.equal('newFoo');
-      expect(data.numberProperty).to.equal(43);
-    }
+      return snapshot.data()!;
+    });
   });
+
+  it('DocumentReference.set() fails to compile if AppModelType argument is missing properties', () =>
+    neverCall(async docRef => {
+      const converter = fakeConverter<{foo: string}, {}>();
+      const docRefWithConverter = docRef.withConverter(converter);
+      // @ts-expect-error The `foo` property declared in AppModelType is missing.
+      await docRefWithConverter.set({});
+    }));
+
+  it('DocumentReference.set() fails to compile if AppModelType argument contains undeclared properties', () =>
+    neverCall(async docRef => {
+      const converter = fakeConverter<{foo: string}, {bar: number}>();
+      const docRefWithConverter = docRef.withConverter(converter);
+      // @ts-expect-error The `bar` property is not declared in AppModelType.
+      await docRefWithConverter.set({foo: 'foo', bar: 42});
+    }));
+
+  it('DocumentReference.set() fails to compile if AppModelType argument contains a property with an incorrect type', () =>
+    neverCall(async docRef => {
+      const converter = fakeConverter<{foo: string}, {foo: number}>();
+      const docRefWithConverter = docRef.withConverter(converter);
+      // @ts-expect-error The `foo` property is declared as `string` in
+      //  AppModelType, but a `number` is specified.
+      await docRefWithConverter.set({foo: 42});
+    }));
+
+  it('DocumentReference.update() successfully compiles even if DbModelType argument is missing properties', () =>
+    neverCall(async docRef => {
+      const converter = fakeConverter<{foo: string}, {bar: number}>();
+      const docRefWithConverter = docRef.withConverter(converter);
+      await docRefWithConverter.update({});
+    }));
+
+  it('DocumentReference.update() fails to compile if DbModelType argument contains undeclared properties', () =>
+    neverCall(async docRef => {
+      const converter = fakeConverter<{foo: string}, {bar: number}>();
+      const docRefWithConverter = docRef.withConverter(converter);
+      // @ts-expect-error The `foo` property is not declared in DbModelType.
+      await docRefWithConverter.update({foo: 'foo', bar: 42});
+    }));
+
+  it('DocumentReference.update() fails to compile if DbModelType argument contains a property with an incorrect type', () =>
+    neverCall(async docRef => {
+      const converter = fakeConverter<{foo: string}, {foo: number}>();
+      const docRefWithConverter = docRef.withConverter(converter);
+      // @ts-expect-error The `foo` property is declared as `number` in
+      //  DbModelType, but a `string` is specified.
+      await docRefWithConverter.update({foo: 'foo'});
+    }));
+
+  it('DocumentReference.get() returns AppModelType', () =>
+    neverCall<Promise<{foo: string}>>(async docRef => {
+      const converter = fakeConverter<{foo: string}, {bar: number}>();
+      const docRefWithConverter = docRef.withConverter(converter);
+      const snapshot = await docRefWithConverter.get();
+      return snapshot.data()!;
+    }));
 });
+
+/**
+ * Does nothing; however, this function can be useful in tests that only check
+ * the compile-time behavior of the TypeScript compiler. For example, a test
+ * that ensures that a certain statement successfully compiles could pass the
+ * code block to this function to exercise the compiler but the code will not
+ * actually be executed at runtime.
+ */
+function neverCall<T>(_: (docRef: DocumentReference) => T): void {
+  _; // Trick eslint into thinking that `_` is used.
+}
+
+/**
+ * Does nothing; this function does not actually exist but is merely _declared_
+ * to exist. This facilitates creating variables typed as FirestoreDataConverter
+ * with the given type parameters at compile time. This can be useful in tests
+ * that only check compile-time behavior of the TypeScript compiler but don't
+ * actually get executed at runtime.
+ */
+declare function fakeConverter<
+  AppModelType,
+  DbModelType extends DocumentData,
+>(): FirestoreDataConverter<AppModelType, DbModelType>;

types/firestore.d.ts

@@ -150,6 +150,23 @@ declare namespace FirebaseFirestore {
    * Using the converter allows you to specify generic type arguments when
    * storing and retrieving objects from Firestore.
    *
+   * In this context, an "AppModel" is a class that is used in an application to
+   * package together related information and functionality. Such a class could,
+   * for example, have properties with complex, nested data types, properties
+   * used for memoization, properties of types not supported by Firestore (such
+   * as `symbol` and `bigint`), and helper functions that perform compound
+   * operations. Such classes are not suitable and/or possible to store into a
+   * Firestore database. Instead, instances of such classes need to be converted
+   * to "plain old JavaScript objects" (POJOs) with exclusively primitive
+   * properties, potentially nested inside other POJOs or arrays of POJOs. In
+   * this context, this type is referred to as the "DbModel" and would be an
+   * object suitable for persisting into Firestore. For convenience,
+   * applications can implement `FirestoreDataConverter` and register the
+   * converter with Firestore objects, such as `DocumentReference` or `Query`,
+   * to automatically convert `AppModel` to `DbModel` when storing into
+   * Firestore, and convert `DbModel` to `AppModel` when retrieving from
+   * Firestore.
+   *
    * @example
    *
    * Simple Example
@@ -353,8 +370,12 @@ declare namespace FirebaseFirestore {
      * `snapshot.data()`.
      *
      * Generally, the data returned from `snapshot.data()` can be cast to
-     * `DbModelType`; however, this is not guaranteed as writes to the database
-     * may have occurred without a type converter enforcing this specific layout.
+     * `DbModelType`; however, this is not guaranteed because Firestore does not
+     * enforce a schema on the database. For example, writes from a previous
+     * version of the application or writes from another client that did not use
+     * a type converter could have written data with different properties and/or
+     * property types. The implementation will need to choose whether to
+     * gracefully recover from non-conforming data or throw an error.
      */
     fromFirestore(snapshot: QueryDocumentSnapshot): AppModelType;
   }