Enable strictPropertyInitialization for Firestore #2078

mikelehen · 2019-08-12T16:30:14Z

This "fixes" all class members that were left uninitialized by the class constructor, allowing us to enable "strictPropertyInitialization" in our tsconfig.json.

I have tried to do this as a series of commits that can be reviewed individually, with the less trivial commits at the beginning. If you'd like me to separate some of these out into separate PRs, let me know.

cc/ @hsubox76 This replaces #1941. I borrowed your fixes where applicable (thanks!), but there were a number of places where I wanted to do a deeper fix to avoid suppressions or unnecessarily making things nullable, etc.

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

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

Fixes a strict-property-initialization violation.

Throw errors instead of potentially returning undefined. I ran the SortedMap perf tests before/after this change and there was no discernible difference.

…efined.

…ed to be initialized. * FieldValue.typeOrder * Mutation.type / key / precondition * PersistenceTransaction.currentSequenceNumber

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

mikelehen · 2019-08-12T16:57:06Z

Note: I had to diffbase this on #2066 because otherwise our tests fail to build/run due to strict violations in the emulator startup code.

schmidt-sebastian · 2019-08-12T17:18:35Z

packages/firestore/src/model/path.ts


  get length(): number {
    return this.len;
  }

-  isEqual(other: Path): boolean {
-    return Path.comparator(this, other) === 0;
+  isEqual(other: this): boolean {


Should this now also be B?

Sure. That makes it so you can't use .isEqual() to check for equality between a ResourcePath and a FieldPath but that's probably good. 👍

schmidt-sebastian · 2019-08-12T17:19:17Z

packages/firestore/src/model/path.ts

    const segments = this.segments.slice(this.offset, this.limit());
-    if (nameOrPath instanceof Path) {
+    if (nameOrPath instanceof BasePath) {


Maybe make this nameOrPath instanceof this.constructor. Or leave as is, if you don't like regressions :)

Neat trick, but that breaks TypeScript's type narrowing (so nameOrPath.foreach() doesn't work since TypeScript still thinks it may be a string.

But I was able to change nameOrPath to be string | B instead of string | this which gives us better compile-time type-safety.

schmidt-sebastian · 2019-08-12T17:23:59Z

packages/firestore/src/api/database.ts

-      !!this._config.settings.host,
-      'FirestoreSettings.host cannot be falsey'
-    );
+    assert(!!this._settings.host, 'FirestoreSettings.host cannot be falsey');


s/cannot be falsey/is not set/ if you want :)

schmidt-sebastian · 2019-08-12T17:29:10Z

packages/firestore/src/api/database.ts

+    return this._firestoreClient.start(persistenceSettings);
+  }
+
+  private static makeDataConverter(databaseId: DatabaseId): UserDataConverter {


I think I would like "createDataConverter" or "newDataConverter" slightly better.

But... could we just make this a non-static function? As long as we return the UserDataConverter, we can still assign it in the constructor.

schmidt-sebastian · 2019-08-12T17:40:17Z

packages/firestore/src/local/memory_persistence.ts

-  private orphanedDocuments: Set<DocumentKey>;
+  private inMemoryPins: ReferenceSet | null = null;
+  private _orphanedDocuments: Set<DocumentKey> | null = null;
+  private get orphanedDocuments(): Set<DocumentKey> {


Nit: Move this below the constructor.

schmidt-sebastian · 2019-08-12T17:54:22Z

packages/firestore/src/remote/persistent_stream.ts

@@ -677,7 +677,7 @@ export class PersistentWriteStream extends PersistentStream<
   * PersistentWriteStream manages propagating this value from responses to the
   * next request.
   */
-  lastStreamToken: ProtoByteString;
+  lastStreamToken: ProtoByteString = '';


Can we use emptyByteString() here?

firebase-js-sdk/packages/firestore/src/platform/platform.ts

Line 85 in df1b588

export function emptyByteString(): ProtoByteString {

Yes, good call!

schmidt-sebastian · 2019-08-12T17:59:32Z

packages/firestore/src/util/sorted_map.ts

-  color: boolean;
-  left: LLRBNode<K, V>;
-  right: LLRBNode<K, V>;
+  get key(): K {


You could change the return type of all of these to "never"

Looks like you're right. Done!

mikelehen

Good suggestions. Thanks!

mikelehen · 2019-08-12T20:09:47Z

packages/firestore/src/model/path.ts


  get length(): number {
    return this.len;
  }

-  isEqual(other: Path): boolean {
-    return Path.comparator(this, other) === 0;
+  isEqual(other: this): boolean {


Sure. That makes it so you can't use .isEqual() to check for equality between a ResourcePath and a FieldPath but that's probably good. 👍

mikelehen · 2019-08-12T20:12:58Z

packages/firestore/src/model/path.ts

    const segments = this.segments.slice(this.offset, this.limit());
-    if (nameOrPath instanceof Path) {
+    if (nameOrPath instanceof BasePath) {


Neat trick, but that breaks TypeScript's type narrowing (so nameOrPath.foreach() doesn't work since TypeScript still thinks it may be a string.

But I was able to change nameOrPath to be string | B instead of string | this which gives us better compile-time type-safety.

mikelehen · 2019-08-12T20:14:04Z

packages/firestore/src/api/database.ts

-      !!this._config.settings.host,
-      'FirestoreSettings.host cannot be falsey'
-    );
+    assert(!!this._settings.host, 'FirestoreSettings.host cannot be falsey');


mikelehen · 2019-08-12T20:24:16Z

packages/firestore/src/local/memory_persistence.ts

-  private orphanedDocuments: Set<DocumentKey>;
+  private inMemoryPins: ReferenceSet | null = null;
+  private _orphanedDocuments: Set<DocumentKey> | null = null;
+  private get orphanedDocuments(): Set<DocumentKey> {


mikelehen · 2019-08-12T20:25:05Z

packages/firestore/src/remote/persistent_stream.ts

@@ -677,7 +677,7 @@ export class PersistentWriteStream extends PersistentStream<
   * PersistentWriteStream manages propagating this value from responses to the
   * next request.
   */
-  lastStreamToken: ProtoByteString;
+  lastStreamToken: ProtoByteString = '';


Yes, good call!

mikelehen · 2019-08-12T20:37:29Z

packages/firestore/src/api/database.ts

+    return this._firestoreClient.start(persistenceSettings);
+  }
+
+  private static makeDataConverter(databaseId: DatabaseId): UserDataConverter {


mikelehen · 2019-08-12T20:37:45Z

packages/firestore/src/util/sorted_map.ts

-  color: boolean;
-  left: LLRBNode<K, V>;
-  right: LLRBNode<K, V>;
+  get key(): K {


Looks like you're right. Done!

…lization

schmidt-sebastian

Thanks for the changes. LGTM.

#2078 altered the initial state of currentUser from undefined to not undefined, which caused the if statement to unconditionally evaluate to true. This results in a race condition that could cause some initial writes to the database to be dropped. Fixes #2135

* Avoid auth triggers if we haven't yet received the initial user. #2078 altered the initial state of currentUser from undefined to not undefined, which caused the if statement to unconditionally evaluate to true. This results in a race condition that could cause some initial writes to the database to be dropped. Fixes #2135 * CHANGELOG update. * Fix to be more like android

Michael Lehenbauer added 14 commits August 12, 2019 09:27

Enable strict mode for scripts/emulator-testing/.

ecfff13

Enable strictPropertyInitialization: Refactor Path to BasePath<T> sim…

0eabdca

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

Remove dummy FirebaseConfig class.

b8d44fe

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

Initialize _dataConverter in constructor.

6b934e3

Fixes a strict-property-initialization violation.

Fix or suppress uninitialized properties in IndexedDbPersistence.

ed5c30f

Fix uninitialized properties in MemoryPersistence.

616e642

Fix uninitialized LLRBEmptyNode properties.

12835a7

Throw errors instead of potentially returning undefined. I ran the SortedMap perf tests before/after this change and there was no discernible difference.

Fix StreamBridge property types to correctly indicate they can be und…

060076d

…efined.

Make sure abstract class members are marked abstract so they don't ne…

ba28df5

…ed to be initialized. * FieldValue.typeOrder * Mutation.type / key / precondition * PersistenceTransaction.currentSequenceNumber

Fix misc. uninitialized properties.

9068114

Suppress some intentionally uninitialized properties.

272a398

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.

a670d75

Suppress some intentionally uninitialized properties in tests.

b5fc74a

Enable strictPropertyInitialization.

45dd960

mikelehen added api: firestore type: cleanup labels Aug 12, 2019

mikelehen assigned mikelehen and schmidt-sebastian and unassigned mikelehen Aug 12, 2019

schmidt-sebastian reviewed Aug 12, 2019

View reviewed changes

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Aug 12, 2019

Minor CR feedback.

43cf111

mikelehen commented Aug 12, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Aug 12, 2019

Merge branch 'master' into mikelehen/firestore-strict-property-initia…

d95b3ac

…lization

mikelehen changed the base branch from mikelehen/emulator-strict to master August 12, 2019 20:58

schmidt-sebastian approved these changes Aug 12, 2019

View reviewed changes

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Aug 12, 2019

mikelehen merged commit 56cbbfd into master Aug 12, 2019

mikelehen deleted the mikelehen/firestore-strict-property-initialization branch August 12, 2019 21:51

mikelehen mentioned this pull request Aug 12, 2019

Turn on Typescript strict option for Firestore #1941

Closed

rsgowman mentioned this pull request Sep 3, 2019

Avoid auth triggers if we haven't yet received the initial user. #2137

Merged

firebase locked and limited conversation to collaborators Oct 8, 2019

Enable strictPropertyInitialization for Firestore #2078

Enable strictPropertyInitialization for Firestore #2078

Uh oh!

Conversation

mikelehen commented Aug 12, 2019

Uh oh!

mikelehen commented Aug 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!