Finish FieldValue migration #2766
Conversation
| @@ -17,8 +17,10 @@ | |||
|
|
|||
| import * as firestore from '@firebase/firestore-types'; | |||
|
|
|||
| import * as api from '../protos/firestore_proto_api'; | |||
There was a problem hiding this comment.
I thought you wanted to get rid of these * imports. Or will we tackle that in a separate PR?
In this case I think you could make this without causing too much pain.
import { Value } from '../protos/firestore_proto_api';This seems like it applies throughout.
There was a problem hiding this comment.
.d.ts files actually contain no JavaScript code and don't affect bundle size. We can remove these imports for consistency, but that leads to the terribly non-descriptive names of Value and Timestamp (unless we rename on import). I vote we keep this as is for now and discuss in a follow up.
| } else if (typeof value === 'string') { | ||
| return new StringValue(value); | ||
| return { stringValue: '' + value }; |
There was a problem hiding this comment.
If typeof value === 'string', do we need to concatenate it with the empty string?
There was a problem hiding this comment.
No. Removed.
| } else if ('booleanValue' in value) { | ||
| return value.booleanValue!; | ||
| } else if ('integerValue' in value) { | ||
| return Number(value.integerValue!); |
There was a problem hiding this comment.
This seems like it should call normalizeNumber rather than just assuming it knows how to do this.
| if (this.timestampsInSnapshots) { | ||
| return timestamp; | ||
| } else { | ||
| return timestamp.toDate(); | ||
| } | ||
| } | ||
|
|
||
| private convertReference(value: RefValue): DocumentReference<T> { | ||
| const key = value.value(); | ||
| const database = this.firestore.ensureClientConfigured().databaseId(); |
There was a problem hiding this comment.
Losing this error message seems like a bad idea. Developers won't have any indication that this isn't going to work without this, no?
There was a problem hiding this comment.
I added this back. I was trying to limit the parsing code for references, since it is already pretty redundant in our code.
| @@ -1,6 +1,6 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2020 Google Inc. | |||
| * Copyright 2017 Google Inc. | |||
There was a problem hiding this comment.
Google LLC. See go/copyright
| } | ||
| const baseValue = this.computeBaseValue(previousValue); | ||
| const sum = this.asNumber(baseValue) + this.asNumber(this.operand); | ||
| return this.serializer.toNumber(sum); |
There was a problem hiding this comment.
Does this give the same result for all possible integer values? It seems like if you add to a sum that's larger than isSafeInteger(sum) this will now return a doubleValue where it used to be an integerValue. Though the calculation lost precision, I'm not sure changing the type is appropriate.
There was a problem hiding this comment.
I didn't think about that. We should not coerce these values into something different. I am, however, not sure how to test this, since we don't seem to have a way to directly unit test the proto output (even LocalStoreTests uses the resulting JavaScript primitives).
| } | ||
| } | ||
|
|
||
| function coercedFieldValuesArray(value: api.Value | null): api.Value[] { | ||
| return value?.arrayValue?.values?.slice() || []; |
There was a problem hiding this comment.
return isArray(value)
? value.arrayValue.values!.slice()
: [];Will likely result in less generated code.
There was a problem hiding this comment.
Yeah - it is pretty horrendous:
function coercedFieldValuesArray(t) {
var e, n, r;
return (null === (r = null === (n = null === (e = t) || void 0 === e ? void 0 : e.arrayValue) || void 0 === n ? void 0 : n.values) || void 0 === r ? void 0 : r.slice()) || [];
}
Note that values is not always set, even when we have an ArrayValue.
| ); | ||
| } else { | ||
| // Server timestamps come after all concrete timestamps. | ||
| return 1; |
There was a problem hiding this comment.
You know, if we just made ServerTimestampValue its own entry in the TypeOrder enum we wouldn't have to special case this at all: any Timestamp to ServerTimestamp comparison would be handled automatically by the way we handle differing types.
There was a problem hiding this comment.
That's @mikelehen level genius. I love it.
There was a problem hiding this comment.
WHAT DID I DO!?!?!?
Hi guys!
| export function contains(haystack: api.ArrayValue, needle: api.Value): boolean { | ||
| return (haystack.values || []).find(v => equals(v, needle)) !== undefined; | ||
| } | ||
|
|
||
| export function compare(left: api.Value, right: api.Value): number { |
There was a problem hiding this comment.
Reading through calling code I fear that names like compare and equals are too generic for free functions that don't operate on general arguments.
What do you think of naming these functions with a type-operation naming scheme? This would be valueCompare, etc.
WDYT?
There was a problem hiding this comment.
Yeah, we can do that. I looked into removing the namespace import for our logging already, and logWarn was what I picked there as a temporary solution.
| return isNegativeZero(n1) === isNegativeZero(n2); | ||
| } else { | ||
| // NaN == NaN | ||
| return n1 !== n1 && n2 !== n2; |
There was a problem hiding this comment.
Elsewhere in this file you're calling isNaN. Make these consistent?
| } | ||
|
|
||
| /** Returns true if `value` is either an IntegerValue . */ | ||
| export function isDobule( |
There was a problem hiding this comment.
s/isDobule/isDouble/
Also, comment is out of date.
There was a problem hiding this comment.
Fixed (also renamed contains to arrayValueContains)
| @@ -578,9 +559,23 @@ export function refValue(databaseId: DatabaseId, key: DocumentKey): api.Value { | |||
| }; | |||
| } | |||
|
|
|||
| /** Returns true if `value` is either an IntegerValue . */ | |||
There was a problem hiding this comment.
Remove "either" from the comment.
schmidt-sebastian
changed the base branch from
mrschmidt/moveprotofieldvalue
to
mrschmidt/rewritefieldvalue
This finishes up the field value migration.
useProto3Jsonby adding it to Platform and plumbing it through to the serializer.Note: This PR is against a branch that does nothing but delete the old field_value.ts and replace it with proto_field_value.ts
SDK Size Before:
317762 firebase-firestore.js
274464 firebase-firestore.memory.js
SDK Size After:
316174 firebase-firestore.js
272668 firebase-firestore.memory.js