-
Notifications
You must be signed in to change notification settings - Fork 147
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: save negative zero as doubleValue #1639
Conversation
Thanks for the PR! Could you (a) sign the CLA (see |
You can ignore the "Auto-approve.yml check" failed check. This looks good to me, but I'm going to get my colleague with more experience in this particular GitHub repository to provide formal approval. Thanks for taking the time to submit this fix! |
@schmidt-sebastian Can you take a look at this PR? It LGTM but wanted a 2nd opinion. |
Awesome. Thank you for the quick review! 馃槉 |
Why?
Javascript only has a number type while firestore differentiates between int64 and double. Both the Web SDK and the nodejs SDK identify integers to store them appropriately. However, the algorithms between the Web SDK and the nodejs SDK differ in one particular case: the negative zero double value.
Relevant lines of code from the JS SDK:
https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/remote/number_serializer.ts#L56
https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/util/types.ts#L44
This PR aligns the nodejs SDK behavior with the one in the Web SDK.
Template
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Appropriate docs were updated (if necessary)Fixes #1638 馃