Untangle FieldValues #3001
Untangle FieldValues #3001
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/userdatareader
branch
from
4cb9866 to
df25951
Compare
Binary Size ReportAffected SDKs
Test Logs
|
| @@ -2586,3 +2561,7 @@ export const PublicCollectionReference = makeConstructorPrivate( | |||
| CollectionReference, | |||
| 'Use firebase.firestore().collection() instead.' | |||
| ); | |||
| export const PublicFieldValue = makeConstructorPrivate( | |||
There was a problem hiding this comment.
Moved here since want to use FieldValue in the RestWrapper, but not pull in these constants as they break tree-shaking.
| @@ -499,28 +496,6 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService { | |||
| return this._firestoreClient.start(componentProvider, persistenceSettings); | |||
| } | |||
|
|
|||
| private createDataReader(databaseId: DatabaseId): UserDataReader { | |||
There was a problem hiding this comment.
As alluded to in the PR transcription, UserDataReader now knows how to deal with DocumentReferences and the pre-converter is removed. This matches Android.
| // update() since we need access to the Firestore instance. | ||
| return new ArrayRemoveFieldValueImpl(elements); | ||
| toFieldTransform(context: ParseContext): null { | ||
| if (context.dataSource === UserDataSource.MergeSet) { |
There was a problem hiding this comment.
All toFieldTransform code is copied from UserDataReader.
|
|
||
| isEqual(other: FieldValue): boolean { | ||
| // TODO(mrschmidt): Implement isEquals | ||
| return false; |
There was a problem hiding this comment.
I don't think our isEqual ever worked with more complex field values since we used instance equality.
| } | ||
| /** Singleton instance. */ | ||
| static instance = new DeleteFieldValueImpl(); |
There was a problem hiding this comment.
These constants break tree-shaking.
| private constructor() { | ||
| super('FieldValue.serverTimestamp'); | ||
| isEqual(other: FieldValue): boolean { | ||
| return other instanceof ServerTimestampFieldValueImpl; |
There was a problem hiding this comment.
Instanceof check since there can now be more than one ServerTimestampFieldValueImpl.
| @@ -140,22 +129,34 @@ function isWrite(dataSource: UserDataSource): boolean { | |||
| } | |||
| } | |||
|
|
|||
| interface ParseSettings { | |||
There was a problem hiding this comment.
This type is new. It basically bundles the arguments from ParseContext and allows for easier manipulation (using the new contextWith method).
| export class Datastore { | ||
| // Make sure that the structural type of `Datastore` is unique. | ||
| // See https://github.com/microsoft/TypeScript/issues/5451 | ||
| private _ = undefined; |
There was a problem hiding this comment.
Unrelated fix for compile time woes in RestWrapper... I can explain more if the GitHub issue isn't helpful. The goal is to make sure that methods that take Datastore don't accept {}.
| this.methodName, | ||
| childPath, | ||
| /*arrayElement=*/ false, | ||
| get path(): FieldPath | null { |
There was a problem hiding this comment.
Just a thought... would it make sense to instead just change the visibility of this.settings to "public" and make the variables of ParseSettings readonly?
There was a problem hiding this comment.
I removed some of them, but kept the ones that are used from other modules (mostly for style). I also moved DatabaseId and Serializer back into the class. These are never mutated and this allows me to get rid of the getter.
| dataSource, | ||
| databaseId: this.databaseId, | ||
| serializer: this.serializer, | ||
| methodName, |
There was a problem hiding this comment.
Is there a reason that the key name was omitted for "methodName"? If not, please add it in for readability and robustness.
There was a problem hiding this comment.
There is a lint setting in our repo that prohibits non-shorthand property notations if they are possible.
E.g. { foo1 : foo } is allowed but since { foo: foo } is the same as { foo } we are meant to use { foo }
See also https://alligator.io/js/object-property-shorthand-es6/
There was a problem hiding this comment.
TIL about "Object Property Value Shorthand"!
This PR shaves off 6KB for all users that don't use FieldValue sentinels (in the up and coming tree-shaken world).
It does so by exposing
parseDataas a top-level function using it in the different FieldValue implementations to directly serialize into a FieldTransform. UserDataConverter then no longer needs to know about each specific implementation.In order to not endlessly complicate the construction of UserDataConverter, I also removed the PreConverter, which doesn't exist on Android. It should not be needed since Rollup bundles everything into one single file, which should removes the need to avoid circular dependencies.
(*) Ideally, we should Copybara this build into Google3 to make sure that it doesn't break. Our current build has lots of circular dependencies and works in Google3.
This PR contains a lot of whitespace diff. Append "?w=1" to remove it from the view in GitHub.