-
Notifications
You must be signed in to change notification settings - Fork 884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable strictPropertyInitialization for Firestore #2078
Enable strictPropertyInitialization for Firestore #2078
Conversation
…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.
…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.
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. |
packages/firestore/src/model/path.ts
Outdated
|
||
get length(): number { | ||
return this.len; | ||
} | ||
|
||
isEqual(other: Path): boolean { | ||
return Path.comparator(this, other) === 0; | ||
isEqual(other: this): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this now also be B
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 👍
const segments = this.segments.slice(this.offset, this.limit()); | ||
if (nameOrPath instanceof Path) { | ||
if (nameOrPath instanceof BasePath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make this nameOrPath instanceof this.constructor
. Or leave as is, if you don't like regressions :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
!!this._config.settings.host, | ||
'FirestoreSettings.host cannot be falsey' | ||
); | ||
assert(!!this._settings.host, 'FirestoreSettings.host cannot be falsey'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/cannot be falsey/is not set/ if you want :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh. SGTM.
return this._firestoreClient.start(persistenceSettings); | ||
} | ||
|
||
private static makeDataConverter(databaseId: DatabaseId): UserDataConverter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM.
private orphanedDocuments: Set<DocumentKey>; | ||
private inMemoryPins: ReferenceSet | null = null; | ||
private _orphanedDocuments: Set<DocumentKey> | null = null; | ||
private get orphanedDocuments(): Set<DocumentKey> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Move this below the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -677,7 +677,7 @@ export class PersistentWriteStream extends PersistentStream< | |||
* PersistentWriteStream manages propagating this value from responses to the | |||
* next request. | |||
*/ | |||
lastStreamToken: ProtoByteString; | |||
lastStreamToken: ProtoByteString = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use emptyByteString()
here?
export function emptyByteString(): ProtoByteString { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good call!
color: boolean; | ||
left: LLRBNode<K, V>; | ||
right: LLRBNode<K, V>; | ||
get key(): K { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could change the return type of all of these to "never"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you're right. Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestions. Thanks!
packages/firestore/src/model/path.ts
Outdated
|
||
get length(): number { | ||
return this.len; | ||
} | ||
|
||
isEqual(other: Path): boolean { | ||
return Path.comparator(this, other) === 0; | ||
isEqual(other: this): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 👍
const segments = this.segments.slice(this.offset, this.limit()); | ||
if (nameOrPath instanceof Path) { | ||
if (nameOrPath instanceof BasePath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
!!this._config.settings.host, | ||
'FirestoreSettings.host cannot be falsey' | ||
); | ||
assert(!!this._settings.host, 'FirestoreSettings.host cannot be falsey'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh. SGTM.
private orphanedDocuments: Set<DocumentKey>; | ||
private inMemoryPins: ReferenceSet | null = null; | ||
private _orphanedDocuments: Set<DocumentKey> | null = null; | ||
private get orphanedDocuments(): Set<DocumentKey> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -677,7 +677,7 @@ export class PersistentWriteStream extends PersistentStream< | |||
* PersistentWriteStream manages propagating this value from responses to the | |||
* next request. | |||
*/ | |||
lastStreamToken: ProtoByteString; | |||
lastStreamToken: ProtoByteString = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good call!
return this._firestoreClient.start(persistenceSettings); | ||
} | ||
|
||
private static makeDataConverter(databaseId: DatabaseId): UserDataConverter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM.
color: boolean; | ||
left: LLRBNode<K, V>; | ||
right: LLRBNode<K, V>; | ||
get key(): K { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you're right. Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. LGTM.
* 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
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.