Protobuf-backed FieldValues #2682
Conversation
| } | ||
|
|
||
| private convertValue(value: api.Value): FieldType { | ||
| if ('nullValue' in value) { |
There was a problem hiding this comment.
Question: this seems to involve string comparison. Is there any way to use some sort of an enum instead? (based on the assumption that it might have some, even if minor, effect on performance when converting many values)
There was a problem hiding this comment.
These are key lookups in a ProtobufJS-generated JavaScript object. Unfortunately, we don't control how these are generated. FWIW, back in the days (with an older version of ProtobufJs), we were able to use an enum-based approach.
| } | ||
|
|
||
| private convertReference(value: string): DocumentKey { | ||
| // TODO(mrschmidt): Move `value()` and `convertValue()` to DocumentSnapshot, |
There was a problem hiding this comment.
Should this comment be moved somewhere else? (it refers to value and convertValue, not convertReference)
There was a problem hiding this comment.
It will allow us to fix convertReference. I think this is not the best comment, but it will get removed before we merge.
| if (other instanceof PrimitiveValue) { | ||
| return equals(this.proto, other.proto); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Just to clarify -- why aren't two maps with the same keys and values ever considered equal?
There was a problem hiding this comment.
A map would pass the instanceof PrimitiveValue check and trigger this code: https://github.com/firebase/firebase-js-sdk/blob/mrschmidt/rewritefieldvalue/packages/firestore/src/model/proto_values.ts#L162 (which enforces key/value based equality).
|
|
||
| /** | ||
| * An ObjectValue represents a MapValue in the Firestore Proto and offers the | ||
| * ability to add an remove fields (via the ObjectValueBuilder). |
| /** | ||
| * An ObjectValue represents a MapValue in the Firestore Proto and offers the | ||
| * ability to add an remove fields (via the ObjectValueBuilder). | ||
| */ |
There was a problem hiding this comment.
Optional: consider extending the comment to compare and contrast ObjectValue and PrimitiveValue that is a map.
There was a problem hiding this comment.
That's a good suggestion, but PrimitiveValue is actually just a stepping stone to allow us to gradually remove field values. In the final PR, PrimitiveValue will disappear.
| const timestamp = Timestamp.fromDate(input); | ||
| return { | ||
| timestampValue: { | ||
| seconds: String(timestamp.seconds), |
There was a problem hiding this comment.
Question: why is String necessary here, but not for integerValue above?
There was a problem hiding this comment.
integerValue is defined as string | number, which this should be too. Fixed.
| expectedTypeOrder: TypeOrder | ||
| ): boolean { | ||
| return value !== undefined && typeOrder(value) === expectedTypeOrder; | ||
| ): value is api.Value { |
There was a problem hiding this comment.
Question: how does value is api.Value work as a return type?
There was a problem hiding this comment.
It tells the transpiler that it can assume that value is api.Value if the function returns true. This allows the following:
let valueOrUnknown : api.Value | null | undefined = ...;
if (isType(valueOrUnknown, ...)) {
// valueOrUnknown is api.Value
}
This is all based on the honor system and we could easily fool the transpiler by providing the wrong guarantees.
|
|
||
| /** | ||
| * An Overlay, which contains an update to apply. Can either be Value proto, a | ||
| * ma of Overlay values (to represent additional nesting at the given key) or |
|
|
||
| /** | ||
| * Removes the field at the specified path. If there is no field at the | ||
| * specified path nothing is changed. |
There was a problem hiding this comment.
Ultranit: I think there should be a comma after path.
| * Removes the field at the specified path. If there is no field at the | ||
| * specified path nothing is changed. | ||
| * | ||
| * @param path The field path to remove |
There was a problem hiding this comment.
Ultranit: missing full stop at the end of the sentence.
Port of firebase/firebase-android-sdk#1156