Skip to content

Support -0.0 in Firestore #2751

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

Merged
merged 6 commits into from
Mar 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/firestore/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# Unreleased
- [fixed] Fixed an issue where the number value `-0.0` would lose its sign when
stored in Firestore.

# 1.10.0
- [feature] Implemented `Timestamp.valueOf()` so that `Timestamp` objects can be
compared for relative ordering using the JavaScript arithmetic comparison
operators (#2632).
Expand Down
4 changes: 4 additions & 0 deletions packages/firestore/src/remote/serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,8 @@ export class JsonProtoSerializer {
return { doubleValue: 'Infinity' } as {};
} else if (doubleValue === -Infinity) {
return { doubleValue: '-Infinity' } as {};
} else if (typeUtils.isNegativeZero(doubleValue)) {
return { doubleValue: '-0' } as {};
}
}
return { doubleValue: val.value() };
Expand Down Expand Up @@ -487,6 +489,8 @@ export class JsonProtoSerializer {
return fieldValue.DoubleValue.POSITIVE_INFINITY;
} else if ((obj.doubleValue as {}) === '-Infinity') {
return fieldValue.DoubleValue.NEGATIVE_INFINITY;
} else if ((obj.doubleValue as {}) === '-0') {
return new fieldValue.DoubleValue(-0);
}
}

Expand Down
12 changes: 10 additions & 2 deletions packages/firestore/src/util/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ export function isNullOrUndefined(value: unknown): boolean {
return value === null || value === undefined;
}

/** Returns whether the value represents -0. */
export function isNegativeZero(value: number) : boolean {
// Detect if the value is -0.0. Based on polyfill from
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
return value === -0 && 1 / value === 1 / -0;
}

/**
* Returns whether a value is an integer and in the safe integer range
* @param value The value to test for being an integer and in the safe range
Expand All @@ -35,7 +42,8 @@ export function isSafeInteger(value: unknown): boolean {
return (
typeof value === 'number' &&
Number.isInteger(value) &&
(value as number) <= Number.MAX_SAFE_INTEGER &&
(value as number) >= Number.MIN_SAFE_INTEGER
!isNegativeZero(value) &&
value <= Number.MAX_SAFE_INTEGER &&
value >= Number.MIN_SAFE_INTEGER
);
}
6 changes: 6 additions & 0 deletions packages/firestore/test/integration/api/type.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

customDeepEqual actually does assert that d is -0 with my changes below. If I just make those changes, this test fails.

});
});

it('can read and write array fields', () => {
return withTestDb(persistence, db => {
return expectRoundtrip(db, { array: [1, 'foo', { deep: true }, null] });
Expand Down
15 changes: 8 additions & 7 deletions packages/firestore/test/util/equality_matcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,20 +34,21 @@ export interface Equatable<T> {
*/

function customDeepEqual(left: unknown, right: unknown): boolean {
/**
* START: Custom compare logic
*/
if (typeof left === 'object' && left && 'isEqual' in left) {
return (left as Equatable<unknown>).isEqual(right);
}
if (typeof right === 'object' && right && 'isEqual' in right) {
return (right as Equatable<unknown>).isEqual(left);
}
/**
* END: Custom compare logic
*/
if (left === right) {
return true;
if (left === 0.0 && right === 0.0) {
// Firestore treats -0.0 and +0.0 as not equals, even though JavaScript
// treats them as equal by default. Implemented based on MDN's Object.is()
// polyfill.
return 1 / left === 1 / right;
} else {
return true;
}
}
if (
typeof left === 'number' &&
Expand Down