Support -0.0 in Firestore #2751
Conversation
We have a couple of unit tests that verify that we treat DoubleValue -0.0 distinct from 0.0, but we were never storing -0.0 as a DoubleValue.
packages/firestore/src/util/types.ts
Outdated
| /** | ||
| * Safely checks if the number is NaN. | ||
| */ | ||
| export function safeIsNaN(value: unknown): boolean { |
There was a problem hiding this comment.
Note this function was never used.
|
This doesn't seem to work with persistence. I will take a look. |
|
So it turns out that JSON serialization does not round-trip -0: I instead opted to use a string constant for -0, which seems to work (even with the backend). |
| @@ -46,6 +46,12 @@ function customDeepEqual(left: unknown, right: unknown): boolean { | |||
| /** | |||
| * END: Custom compare logic | |||
| */ | |||
| if (left === 0.0 && right === 0.0) { | |||
There was a problem hiding this comment.
You could probably move this inside the left === right case.
if (left === right) {
if (left === 0) {
// ...
return 1 / left === 1 / right;
} else {
return true;
}
}Also, the comment above about "END: Custom compare logic" I can't say I really understand what it means. It seems like there's custom logic both above and below the comment. Could you strike it or make it clear what it's actually talking about?
There was a problem hiding this comment.
Done. I was debating doing this earlier anyways.
packages/firestore/src/util/types.ts
Outdated
| @@ -58,25 +58,22 @@ export function isNullOrUndefined(value: unknown): boolean { | |||
| return value === null || value === undefined; | |||
| } | |||
|
|
|||
| /** Returns whether the value represents -0.. */ | |||
| @@ -47,6 +47,12 @@ apiDescribe('Firestore', (persistence: boolean) => { | |||
| }); | |||
| }); | |||
|
|
|||
| it('can read and write number fields', () => { | |||
| return withTestDb(persistence, db => { | |||
| return expectRoundtrip(db, { a: 1, b: NaN, c: Infinity, d: -0.0 }); | |||
There was a problem hiding this comment.
This doesn't actually make any assertion about the format of the d value. Is it worth separately asserting that in the forward direction -0.0 renders as a string '-0'?
There was a problem hiding this comment.
customDeepEqual actually does assert that d is -0 with my changes below. If I just make those changes, this test fails.
We have a couple of unit tests that verify that we treat DoubleValue -0.0 distinct from 0.0, but we were never storing -0.0 as a DoubleValue.