Enable strict function types (#1082)

* Strict

* More strictness

* [AUTOMATED]: Prettier Code Styling

* Lint and cleanup

* Use Unknown type

Author: Greg Soltis

packages/firestore/src/local/indexeddb_mutation_queue.ts

@@ -200,7 +200,9 @@ export class IndexedDbMutationQueue implements MutationQueue {
   getLastStreamToken(
     transaction: PersistenceTransaction
   ): PersistencePromise<ProtoByteString> {
-    return PersistencePromise.resolve(this.metadata.lastStreamToken);
+    return PersistencePromise.resolve<ProtoByteString>(
+      this.metadata.lastStreamToken
+    );
   }
 
   setLastStreamToken(

packages/firestore/src/local/persistence_promise.ts

@@ -157,7 +157,7 @@ export class PersistencePromise<T> {
   static resolve(): PersistencePromise<void>;
   static resolve<R>(result: R): PersistencePromise<R>;
   static resolve<R>(result?: R): PersistencePromise<R | void> {
-    return new PersistencePromise<R>((resolve, reject) => {
+    return new PersistencePromise<R | void>((resolve, reject) => {
       resolve(result);
     });
   }

packages/firestore/src/local/simple_db.ts

@@ -19,6 +19,7 @@ import { debug } from '../util/log';
 import { AnyDuringMigration } from '../util/misc';
 import { PersistencePromise } from './persistence_promise';
 import { SCHEMA_VERSION } from './indexeddb_schema';
+import { AnyJs } from '../util/misc';
 import { Deferred } from '../util/promise';
 import { Code, FirestoreError } from '../util/error';
 
@@ -71,7 +72,7 @@ export class SimpleDb {
         );
       };
 
-      request.onerror = (event: ErrorEvent) => {
+      request.onerror = (event: Event) => {
         reject((event.target as IDBOpenDBRequest).error);
       };
 
@@ -147,7 +148,7 @@ export class SimpleDb {
   }
 
   /** Helper to get a typed SimpleDbStore from a transaction. */
-  static getStore<KeyType extends IDBValidKey, ValueType>(
+  static getStore<KeyType extends IDBValidKey, ValueType extends AnyJs>(
     txn: SimpleDbTransaction,
     store: string
   ): SimpleDbStore<KeyType, ValueType> {
@@ -319,7 +320,7 @@ export class SimpleDbTransaction {
    * Note that we can't actually enforce that the KeyType and ValueType are
    * correct, but they allow type safety through the rest of the consuming code.
    */
-  store<KeyType extends IDBValidKey, ValueType>(
+  store<KeyType extends IDBValidKey, ValueType extends AnyJs>(
     storeName: string
   ): SimpleDbStore<KeyType, ValueType> {
     const store = this.transaction.objectStore(storeName);
@@ -338,7 +339,10 @@ export class SimpleDbTransaction {
  * 3) Provides a higher-level API to avoid needing to do excessive wrapping of
  * intermediate IndexedDB types (IDBCursorWithValue, etc.)
  */
-export class SimpleDbStore<KeyType extends IDBValidKey, ValueType> {
+export class SimpleDbStore<
+  KeyType extends IDBValidKey,
+  ValueType extends AnyJs
+> {
   constructor(private store: IDBObjectStore) {}
 
   /**

packages/firestore/src/platform_node/grpc_connection.ts

@@ -210,7 +210,7 @@ export class GrpcConnection implements Connection {
     const grpcStream = rpc();
 
     let closed = false;
-    let close: (err?: Error) => void;
+    let close: (err?: FirestoreError) => void;
 
     const stream = new StreamBridge<Req, Resp>({
       sendFn: (msg: Req) => {

packages/firestore/src/util/async_queue.ts

@@ -16,7 +16,7 @@
 
 import { assert, fail } from './assert';
 import * as log from './log';
-import { AnyJs } from './misc';
+import { Unknown } from './misc';
 import { Deferred, CancelablePromise } from './promise';
 import { Code, FirestoreError } from './error';
 
@@ -60,7 +60,7 @@ export enum TimerId {
  *
  * Supports cancellation (via cancel()) and early execution (via skipDelay()).
  */
-class DelayedOperation<T> implements CancelablePromise<T> {
+class DelayedOperation<T extends Unknown> implements CancelablePromise<T> {
   // handle for use with clearTimeout(), or null if the operation has been
   // executed or canceled already.
   private timerHandle: TimerHandle | null;
@@ -94,13 +94,13 @@ class DelayedOperation<T> implements CancelablePromise<T> {
    *   PORTING NOTE: This exists to prevent making removeDelayedOperation() and
    *   the DelayedOperation class public.
    */
-  static createAndSchedule<T>(
+  static createAndSchedule<R extends Unknown>(
     asyncQueue: AsyncQueue,
     timerId: TimerId,
     delayMs: number,
-    op: () => Promise<T>,
-    removalCallback: (op: DelayedOperation<T>) => void
-  ): DelayedOperation<T> {
+    op: () => Promise<R>,
+    removalCallback: (op: DelayedOperation<R>) => void
+  ): DelayedOperation<R> {
     const targetTime = Date.now() + delayMs;
     const delayedOp = new DelayedOperation(
       asyncQueue,
@@ -177,11 +177,11 @@ class DelayedOperation<T> implements CancelablePromise<T> {
 
 export class AsyncQueue {
   // The last promise in the queue.
-  private tail: Promise<AnyJs | void> = Promise.resolve();
+  private tail: Promise<Unknown> = Promise.resolve();
 
   // Operations scheduled to be queued in the future. Operations are
   // automatically removed after they are run or canceled.
-  private delayedOperations: Array<DelayedOperation<AnyJs>> = [];
+  private delayedOperations: Array<DelayedOperation<Unknown>> = [];
 
   // visible for testing
   failure: Error;
@@ -194,7 +194,7 @@ export class AsyncQueue {
    * Adds a new operation to the queue. Returns a promise that will be resolved
    * when the promise returned by the new operation is (with its value).
    */
-  enqueue<T>(op: () => Promise<T>): Promise<T> {
+  enqueue<T extends Unknown>(op: () => Promise<T>): Promise<T> {
     this.verifyNotFailed();
     const newTail = this.tail.then(() => {
       this.operationInProgress = true;
@@ -233,7 +233,7 @@ export class AsyncQueue {
    * `delayMs` has elapsed. The returned CancelablePromise can be used to cancel
    * the operation prior to its running.
    */
-  enqueueAfterDelay<T>(
+  enqueueAfterDelay<T extends Unknown>(
     timerId: TimerId,
     delayMs: number,
     op: () => Promise<T>
@@ -247,7 +247,7 @@ export class AsyncQueue {
       `Attempted to schedule multiple operations with timer id ${timerId}.`
     );
 
-    const delayedOp = DelayedOperation.createAndSchedule(
+    const delayedOp = DelayedOperation.createAndSchedule<Unknown>(
       this,
       timerId,
       delayMs,
@@ -329,7 +329,7 @@ export class AsyncQueue {
   }
 
   /** Called once a DelayedOperation is run or canceled. */
-  private removeDelayedOperation<T>(op: DelayedOperation<T>): void {
+  private removeDelayedOperation(op: DelayedOperation<Unknown>): void {
     // NOTE: indexOf / slice are O(n), but delayedOperations is expected to be small.
     const index = this.delayedOperations.indexOf(op);
     assert(index >= 0, 'Delayed operation not found.');

packages/firestore/src/util/misc.ts

@@ -25,6 +25,13 @@ export type EventHandler<E> = (value: E) => void;
  */
 export type AnyJs = null | undefined | boolean | number | string | object;
 
+/**
+ * `Unknown` is a stand-in for Typescript 3's `unknown` type. It is similar to
+ * `any` but forces code to check types before performing operations on a value
+ * of type `Unknown`. See: https://blogs.msdn.microsoft.com/typescript/2018/07/30/announcing-typescript-3-0/#the-unknown-type
+ */
+export type Unknown = null | undefined | {} | void;
+
 // TODO(b/66916745): AnyDuringMigration was used to suppress type check failures
 // that were found during the upgrade to TypeScript 2.4. They need to be audited
 // and fixed.

packages/firestore/test/unit/local/indexeddb_schema.test.ts

@@ -53,7 +53,7 @@ function withDb(
     request.onsuccess = (event: Event) => {
       resolve((event.target as IDBOpenDBRequest).result);
     };
-    request.onerror = (event: ErrorEvent) => {
+    request.onerror = (event: Event) => {
       reject((event.target as IDBOpenDBRequest).error);
     };
   })

packages/firestore/test/unit/local/persistence_promise.test.ts

@@ -198,7 +198,7 @@ describe('PersistencePromise', () => {
       })
     );
     updates.push(
-      async(1).next(x => {
+      async(1).next<void>(x => {
         throw error;
       })
     );

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -87,7 +87,8 @@ import {
   path,
   setMutation,
   TestSnapshotVersion,
-  version
+  version,
+  expectFirestoreError
 } from '../../util/helpers';
 
 class MockConnection implements Connection {
@@ -308,9 +309,9 @@ class EventAggregator implements Observer<ViewSnapshot> {
     });
   }
 
-  error(error: FirestoreError): void {
-    expect(error.code).to.exist;
-    this.pushEvent({ query: this.query, error });
+  error(error: Error): void {
+    expectFirestoreError(error);
+    this.pushEvent({ query: this.query, error: error as FirestoreError });
   }
 }
 
@@ -913,8 +914,7 @@ abstract class TestRunner {
     const expectedQuery = this.parseQuery(expected.query);
     expect(actual.query).to.deep.equal(expectedQuery);
     if (expected.errorCode) {
-      // TODO(dimond): better matcher
-      expect(actual.error instanceof Error).to.equal(true);
+      expectFirestoreError(actual.error);
     } else {
       const expectedChanges: DocumentViewChange[] = [];
       if (expected.removed) {

packages/firestore/test/util/helpers.ts

@@ -555,9 +555,9 @@ export function expectEqualArrays(
  * Checks that an ordered array of elements yields the correct pair-wise
  * comparison result for the supplied comparator
  */
-export function expectCorrectComparisons(
-  array: AnyJs[],
-  comp: (left: AnyJs, right: AnyJs) => number
+export function expectCorrectComparisons<T extends AnyJs>(
+  array: T[],
+  comp: (left: T, right: T) => number
 ): void {
   for (let i = 0; i < array.length; i++) {
     for (let j = 0; j < array.length; j++) {
@@ -584,9 +584,9 @@ export function expectCorrectComparisons(
  * returns the same as comparing the indexes of the "equality groups"
  * (0 for items in the same group).
  */
-export function expectCorrectComparisonGroups(
-  groups: AnyJs[][],
-  comp: (left: AnyJs, right: AnyJs) => number
+export function expectCorrectComparisonGroups<T extends AnyJs>(
+  groups: T[][],
+  comp: (left: T, right: T) => number
 ): void {
   for (let i = 0; i < groups.length; i++) {
     for (const left of groups[i]) {
@@ -674,3 +674,7 @@ export function size(obj: JsonObject<AnyJs>): number {
   forEach(obj, () => c++);
   return c;
 }
+
+export function expectFirestoreError(err: Error): void {
+  expect(err.name).to.equal('FirebaseError');
+}

packages/firestore/tsconfig.json

@@ -1,7 +1,8 @@
 {
   "extends": "../../config/tsconfig.base.json",
   "compilerOptions": {
-    "outDir": "dist"
+    "outDir": "dist",
+    "strictFunctionTypes": true
   },
   "exclude": [
     "dist/**/*"