Skip to content
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

feat: implement Timestamp.valueOf() (#944) #947

Merged
merged 4 commits into from Mar 4, 2020

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Mar 2, 2020

This enables Timestamp objects to be compared for relative ordering using the <, <=, >, and >= operators.

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #944

This enables Timestamp objects to be compared for relative ordering using the <, <=, >, and >= operators.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 2, 2020
*
* @type {number}
*/
const MIN_SECONDS = -62135596800;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm that the backend rejects all older timestamps, as well as any timestamps past the year 10,000?

If that is the case, then we should add validation to the constructor. We need to make sure that valueOf() is never invoked on invalid data. If that is not the case, then we should go back to the drawing board and come up with a strategy.

I am also slightly inclined to ship this as part of the Node 8 deprecation that is around the corner, in which case we can make this a proper breaking change. The PR title would then be "feat!: ...".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it out and an exception is thrown when invalid seconds are specified. The message looks like this:

Error: 3 INVALID_ARGUMENT: Timestamp seconds exceeds limit for field.

I wrote a little app to test this out https://github.com/dconeybe/node-timestamp-range-test.

I like the idea of adding the validation to the constructor and shipping this as part of the Node 8 deprecation. I will update this PR with that logic.

We could even go a step further and make timestamp.ts an exact copy of https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/api/timestamp.ts. There is no reason why these two files (and their accompanying unit tests) should differ in any way. Making them identical would help reduce the maintenance burden. What do you think?

@dconeybe dconeybe changed the title feat: implement Timestamp.valueOf() (#944) feat!: implement Timestamp.valueOf() (#944) Mar 4, 2020
@dconeybe dconeybe changed the title feat!: implement Timestamp.valueOf() (#944) feat: implement Timestamp.valueOf() (#944) Mar 4, 2020
@dconeybe dconeybe merged commit 24a96c6 into master Mar 4, 2020
@dconeybe dconeybe deleted the dconeybe/TimestampValueOf branch March 4, 2020 18:48
@angelru angelru mentioned this pull request Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Add valueOf() to Timestamp instances
3 participants