Enable strictPropertyInitialization for Firestore (#2078)

* Enable strictPropertyInitialization: Refactor Path to BasePath<T> similar to Android.

This avoids having uninitialized properties due to the init() method doing the construction.

* Remove dummy FirebaseConfig class.

This class doesn't exist on other platforms and was responsible for a bunch of
strict-property-initialization violations.

* Initialize _dataConverter in constructor.

Fixes a strict-property-initialization violation.

* Fix or suppress uninitialized properties in IndexedDbPersistence.

* Fix StreamBridge property types to correctly indicate they can be undefined.

* Make sure abstract class members are marked abstract so they don't need to be initialized.

* FieldValue.typeOrder
* Mutation.type / key / precondition
* PersistenceTransaction.currentSequenceNumber

* Fix uninitialized LLRBEmptyNode properties.

Throw errors instead of potentially returning undefined.

I ran the SortedMap perf tests before/after this change and there was no discernible difference.

* Fix uninitialized properties in MemoryPersistence.

* Fix misc. uninitialized properties.

* Suppress some intentionally uninitialized properties.

These would be obnoxious to define as `| undefined` but we also can't initialize them in the constructor.

* Fix some misc. uninitialized properties in tests.

* Suppress some intentionally uninitialized properties in tests.

* Enable strictPropertyInitialization.

* Minor CR feedback.

Author: Michael Lehenbauer

packages/firestore/src/api/credentials.ts

@@ -134,7 +134,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
   private tokenListener: ((token: string | null) => void) | null = null;
 
   /** Tracks the current User. */
-  private currentUser: User;
+  private currentUser: User = User.UNAUTHENTICATED;
 
   /**
    * Counter used to detect if the token changed while a getToken request was

packages/firestore/src/api/database.ts

@@ -280,24 +280,18 @@ class FirestoreSettings {
   }
 }
 
-class FirestoreConfig {
-  databaseId: DatabaseId;
-  persistenceKey: string;
-  credentials: CredentialsProvider;
-  firebaseApp: FirebaseApp;
-  settings: FirestoreSettings;
-  persistence: boolean;
-}
-
 /**
  * The root reference to the database.
  */
 export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
   // The objects that are a part of this API are exposed to third-parties as
   // compiled javascript so we want to flag our private members with a leading
   // underscore to discourage their use.
-  private readonly _config: FirestoreConfig;
   readonly _databaseId: DatabaseId;
+  private readonly _persistenceKey: string;
+  private _credentials: CredentialsProvider;
+  private readonly _firebaseApp: FirebaseApp | null = null;
+  private _settings: FirestoreSettings;
 
   // The firestore client instance. This will be available as soon as
   // configureClient is called, but any calls against it will block until
@@ -311,18 +305,17 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
   // TODO(mikelehen): Use modularized initialization instead.
   readonly _queue = new AsyncQueue();
 
-  _dataConverter: UserDataConverter;
+  readonly _dataConverter: UserDataConverter;
 
   constructor(databaseIdOrApp: FirestoreDatabase | FirebaseApp) {
-    const config = new FirestoreConfig();
     if (typeof (databaseIdOrApp as FirebaseApp).options === 'object') {
       // This is very likely a Firebase app object
       // TODO(b/34177605): Can we somehow use instanceof?
       const app = databaseIdOrApp as FirebaseApp;
-      config.firebaseApp = app;
-      config.databaseId = Firestore.databaseIdFromApp(app);
-      config.persistenceKey = config.firebaseApp.name;
-      config.credentials = new FirebaseCredentialsProvider(app);
+      this._firebaseApp = app;
+      this._databaseId = Firestore.databaseIdFromApp(app);
+      this._persistenceKey = app.name;
+      this._credentials = new FirebaseCredentialsProvider(app);
     } else {
       const external = databaseIdOrApp as FirestoreDatabase;
       if (!external.projectId) {
@@ -332,15 +325,14 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
         );
       }
 
-      config.databaseId = new DatabaseId(external.projectId, external.database);
+      this._databaseId = new DatabaseId(external.projectId, external.database);
       // Use a default persistenceKey that lines up with FirebaseApp.
-      config.persistenceKey = '[DEFAULT]';
-      config.credentials = new EmptyCredentialsProvider();
+      this._persistenceKey = '[DEFAULT]';
+      this._credentials = new EmptyCredentialsProvider();
     }
 
-    config.settings = new FirestoreSettings({});
-    this._config = config;
-    this._databaseId = config.databaseId;
+    this._settings = new FirestoreSettings({});
+    this._dataConverter = this.createDataConverter(this._databaseId);
   }
 
   settings(settingsLiteral: firestore.Settings): void {
@@ -358,7 +350,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
     }
 
     const newSettings = new FirestoreSettings(settingsLiteral);
-    if (this._firestoreClient && !this._config.settings.isEqual(newSettings)) {
+    if (this._firestoreClient && !this._settings.isEqual(newSettings)) {
       throw new FirestoreError(
         Code.FAILED_PRECONDITION,
         'Firestore has already been started and its settings can no longer ' +
@@ -367,11 +359,9 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
       );
     }
 
-    this._config.settings = newSettings;
+    this._settings = newSettings;
     if (newSettings.credentials !== undefined) {
-      this._config.credentials = makeCredentialsProvider(
-        newSettings.credentials
-      );
+      this._credentials = makeCredentialsProvider(newSettings.credentials);
     }
   }
 
@@ -416,7 +406,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
 
     return this.configureClient(
       new IndexedDbPersistenceSettings(
-        this._config.settings.cacheSizeBytes,
+        this._settings.cacheSizeBytes,
         synchronizeTabs
       )
     );
@@ -489,30 +479,38 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
 
   private makeDatabaseInfo(): DatabaseInfo {
     return new DatabaseInfo(
-      this._config.databaseId,
-      this._config.persistenceKey,
-      this._config.settings.host,
-      this._config.settings.ssl,
-      this._config.settings.forceLongPolling
+      this._databaseId,
+      this._persistenceKey,
+      this._settings.host,
+      this._settings.ssl,
+      this._settings.forceLongPolling
     );
   }
 
   private configureClient(
     persistenceSettings: InternalPersistenceSettings
   ): Promise<void> {
-    assert(
-      !!this._config.settings.host,
-      'FirestoreSettings.host cannot be falsey'
-    );
+    assert(!!this._settings.host, 'FirestoreSettings.host is not set');
 
     assert(!this._firestoreClient, 'configureClient() called multiple times');
 
     const databaseInfo = this.makeDatabaseInfo();
 
+    this._firestoreClient = new FirestoreClient(
+      PlatformSupport.getPlatform(),
+      databaseInfo,
+      this._credentials,
+      this._queue
+    );
+
+    return this._firestoreClient.start(persistenceSettings);
+  }
+
+  private createDataConverter(databaseId: DatabaseId): UserDataConverter {
     const preConverter = (value: unknown): unknown => {
       if (value instanceof DocumentReference) {
-        const thisDb = this._config.databaseId;
-        const otherDb = value.firestore._config.databaseId;
+        const thisDb = databaseId;
+        const otherDb = value.firestore._databaseId;
         if (!otherDb.isEqual(thisDb)) {
           throw new FirestoreError(
             Code.INVALID_ARGUMENT,
@@ -521,21 +519,12 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
               `for database ${thisDb.projectId}/${thisDb.database}`
           );
         }
-        return new DocumentKeyReference(this._config.databaseId, value._key);
+        return new DocumentKeyReference(databaseId, value._key);
       } else {
         return value;
       }
     };
-    this._dataConverter = new UserDataConverter(preConverter);
-
-    this._firestoreClient = new FirestoreClient(
-      PlatformSupport.getPlatform(),
-      databaseInfo,
-      this._config.credentials,
-      this._queue
-    );
-
-    return this._firestoreClient.start(persistenceSettings);
+    return new UserDataConverter(preConverter);
   }
 
   private static databaseIdFromApp(app: FirebaseApp): DatabaseId {
@@ -558,14 +547,14 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
   }
 
   get app(): FirebaseApp {
-    if (!this._config.firebaseApp) {
+    if (!this._firebaseApp) {
       throw new FirestoreError(
         Code.FAILED_PRECONDITION,
         "Firestore was not initialized using the Firebase SDK. 'app' is " +
           'not available'
       );
     }
-    return this._config.firebaseApp;
+    return this._firebaseApp;
   }
 
   INTERNAL = {
@@ -668,7 +657,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
   // Note: this is not a property because the minifier can't work correctly with
   // the way TypeScript compiler outputs properties.
   _areTimestampsInSnapshotsEnabled(): boolean {
-    return this._config.settings.timestampsInSnapshots;
+    return this._settings.timestampsInSnapshots;
   }
 }
 

packages/firestore/src/core/event_manager.ts

@@ -28,8 +28,8 @@ import { DocumentViewChange, ChangeType, ViewSnapshot } from './view_snapshot';
  * tracked by EventManager.
  */
 class QueryListenersInfo {
-  viewSnap: ViewSnapshot | null;
-  targetId: TargetId;
+  viewSnap: ViewSnapshot | null = null;
+  targetId: TargetId = 0;
   listeners: QueryListener[] = [];
 }
 
@@ -166,7 +166,7 @@ export class QueryListener {
 
   private options: ListenOptions;
 
-  private snap: ViewSnapshot;
+  private snap: ViewSnapshot | null = null;
 
   private onlineState: OnlineState = OnlineState.Unknown;
 

packages/firestore/src/core/firestore_client.ts

@@ -99,18 +99,18 @@ export class FirestoreClient {
   // initialization completes before any other work is queued, we're cheating
   // with the types rather than littering the code with '!' or unnecessary
   // undefined checks.
-  private eventMgr: EventManager;
-  private persistence: Persistence;
-  private localStore: LocalStore;
-  private remoteStore: RemoteStore;
-  private syncEngine: SyncEngine;
+  private eventMgr!: EventManager;
+  private persistence!: Persistence;
+  private localStore!: LocalStore;
+  private remoteStore!: RemoteStore;
+  private syncEngine!: SyncEngine;
+  // PORTING NOTE: SharedClientState is only used for multi-tab web.
+  private sharedClientState!: SharedClientState;
+
   private lruScheduler?: LruScheduler;
 
   private readonly clientId = AutoId.newId();
 
-  // PORTING NOTE: SharedClientState is only used for multi-tab web.
-  private sharedClientState: SharedClientState;
-
   constructor(
     private platform: Platform,
     private databaseInfo: DatabaseInfo,

packages/firestore/src/core/sync_engine.ts

@@ -110,7 +110,7 @@ class LimboResolution {
    * decide whether it needs to manufacture a delete event for the target once
    * the target is CURRENT.
    */
-  receivedDocument: boolean;
+  receivedDocument: boolean = false;
 }
 
 /**

packages/firestore/src/core/target_id_generator.ts

@@ -42,7 +42,8 @@ enum GeneratorIds {
 // TODO(mrschmidt): Explore removing this class in favor of generating these IDs
 // directly in SyncEngine and LocalStore.
 export class TargetIdGenerator {
-  private nextId: TargetId;
+  // Initialized in the constructor via call to seek().
+  private nextId!: TargetId;
 
   /**
    * Instantiates a new TargetIdGenerator. If a seed is provided, the generator

packages/firestore/src/core/transaction.ts

@@ -40,7 +40,7 @@ export class Transaction {
    * A deferred usage error that occurred previously in this transaction that
    * will cause the transaction to fail once it actually commits.
    */
-  private lastWriteError: FirestoreError;
+  private lastWriteError: FirestoreError | null = null;
 
   /**
    * Set of documents that have been written in the transaction.

packages/firestore/src/local/indexeddb_persistence.ts

@@ -236,23 +236,28 @@ export class IndexedDbPersistence implements Persistence {
   private readonly document: Document | null;
   private readonly window: Window;
 
-  private simpleDb: SimpleDb;
+  // Technically these types should be `| undefined` because they are
+  // initialized asynchronously by start(), but that would be more misleading
+  // than useful.
+  private simpleDb!: SimpleDb;
+  private listenSequence!: ListenSequence;
+
   private _started = false;
   private isPrimary = false;
   private networkEnabled = true;
   private dbName: string;
 
   /** Our window.unload handler, if registered. */
-  private windowUnloadHandler: (() => void) | null;
+  private windowUnloadHandler: (() => void) | null = null;
   private inForeground = false;
 
   private serializer: LocalSerializer;
 
   /** Our 'visibilitychange' listener if registered. */
-  private documentVisibilityHandler: ((e?: Event) => void) | null;
+  private documentVisibilityHandler: ((e?: Event) => void) | null = null;
 
   /** The client metadata refresh task. */
-  private clientMetadataRefresher: CancelablePromise<void>;
+  private clientMetadataRefresher: CancelablePromise<void> | null = null;
 
   /** The last time we garbage collected the Remote Document Changelog. */
   private lastGarbageCollectionTime = Number.NEGATIVE_INFINITY;
@@ -267,7 +272,6 @@ export class IndexedDbPersistence implements Persistence {
   private readonly indexManager: IndexedDbIndexManager;
   private readonly remoteDocumentCache: IndexedDbRemoteDocumentCache;
   private readonly webStorage: Storage;
-  private listenSequence: ListenSequence;
   readonly referenceDelegate: IndexedDbLruDelegate;
 
   // Note that `multiClientParams` must be present to enable multi-client support while multi-tab
@@ -692,6 +696,7 @@ export class IndexedDbPersistence implements Persistence {
     this.markClientZombied();
     if (this.clientMetadataRefresher) {
       this.clientMetadataRefresher.cancel();
+      this.clientMetadataRefresher = null;
     }
     this.detachVisibilityHandler();
     this.detachWindowUnloadHook();
@@ -1131,7 +1136,7 @@ function clientMetadataStore(
 
 /** Provides LRU functionality for IndexedDB persistence. */
 export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
-  private inMemoryPins: ReferenceSet | null;
+  private inMemoryPins: ReferenceSet | null = null;
 
   readonly garbageCollector: LruGarbageCollector;
 

packages/firestore/src/local/lru_garbage_collector.ts

@@ -217,7 +217,7 @@ const REGULAR_GC_DELAY_MS = 5 * 60 * 1000;
  * whether or not GC is enabled, as well as which delay to use before the next run.
  */
 export class LruScheduler {
-  private hasRun: boolean;
+  private hasRun: boolean = false;
   private gcTask: CancelablePromise<void> | null;
 
   constructor(