Implement Timestamp.valueOf(). #2662

dconeybe · 2020-02-21T20:57:01Z

Implement Timestamp.valueOf() so that Timestamp objects can be compared for relative ordering using the arithmetic comparison operators (i.e. <, <=, >, and >=).

Fixes #2632

…es for the bounds of seconds and nanoseconds.

This enables comparison of Timestamp objects using the arithmetic comparison operators, such as < and >. #2632

schmidt-sebastian

Whee! This is a great PR. Left some feedback to make sure you don't run out of work :)

schmidt-sebastian · 2020-02-21T21:14:42Z

packages/firestore/src/api/timestamp.ts

+  // This will break in the year 10,000.
+  private static readonly MAX_SECONDS = 253402300799;
+  private static readonly MIN_NANOSECONDS = 0;
+  private static readonly MAX_NANOSECONDS = 1e9 - 1;


Note that "private" is not a thing in the final JavaScript build. When TypeScript transpiles this class to JS, these static members essentially become public. To indicate that we want these member to be treated as private members, we generally prefix all private members of our API classes with underscores.

FWIW, you could declare these as constants within the file rather than the class and then they wouldn't be exported at all.

I have moved these variables and the normalizeAndPad() private method to outside the class declaration so that they will be truly private.

schmidt-sebastian · 2020-02-21T21:15:56Z

packages/firestore/src/api/timestamp.ts

@@ -19,6 +19,13 @@ import { Code, FirestoreError } from '../util/error';
 import { primitiveComparator } from '../util/misc';

 export class Timestamp {
+  // Midnight at the beginning of 1/1/1 is the earliest Firestore supports.
+  private static readonly MIN_SECONDS = -62135596800;
+  // This will break in the year 10,000.


Nit: This constant won't break. It will continue to point to the year 10,000 even in the year 10,001.
You can probably just mention that the value corresponds to the year 10,000.

Haha true. I just copied this comment from the code in its old location. I've updated these comments to state the date to which they correspond and removed the mention of "breaking".

schmidt-sebastian · 2020-02-21T21:20:24Z

packages/firestore/src/api/timestamp.ts

+  ): string {
+    const padLength = Math.ceil(Math.log10(maxValue - minValue));
+    const normalizedValue = value - minValue;
+    return normalizedValue.toString().padStart(padLength, '0');


FYI: I started a discussion in the JS room whether we can use this API. See https://caniuse.com/#feat=pad-start-end for context.

The padStart() method is covered by the "required" polyfills documented at https://firebase.google.com/support/guides/environments_js-sdk#polyfills. So I think we're okay.

schmidt-sebastian · 2020-02-21T21:23:15Z

packages/firestore/src/api/timestamp.ts

+    minValue: number,
+    maxValue: number
+  ): string {
+    const padLength = Math.ceil(Math.log10(maxValue - minValue));


Most pad() methods that I am aware of take as an argument the desired length to pad to. I would suggest you do the same thing here: The nanosecond length is always 9, and the MAX_SECONDS value is a constant that you could define as const or via Math.ceil(Math.log10(MAX_SECONDS))).

Thanks for the suggestion. I'll simply hardcode the pad lengths, since there is really no value in computing those constants over and over again.

schmidt-sebastian · 2020-02-21T21:24:20Z

packages/firestore/test/unit/api/timestamp.test.ts

 import { Timestamp } from '../../../src/api/timestamp';
 import { addEqualityMatcher } from '../../util/equality_matcher';

 describe('Timestamp', () => {
  addEqualityMatcher();

+  it('constructor should validate the "seconds" argument and store it.', () => {


Nit: I think in general we don't use period in our test names.

schmidt-sebastian · 2020-02-21T21:32:39Z

Uhm, I think GitHub ate my last comment....

Please also add a test that shows that <, = and > now work correctly. You should also now be able to use Timestamps in Map objects, and object equality should now be based on the Timestamp data rather than the address of the Timestamp object. You can test this like this:

Map m = new Map();
m.add(new Timestamp(1,1));
expect(m.has(new Timestamp(1,1)).to.be.true;

Furthermore, can you also add a Changelog entry: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/CHANGELOG.md

Your PR description should also include "Fixes: " which will auto-close the issue on merging.

dconeybe · 2020-02-26T18:42:49Z

Uhm, I think GitHub ate my last comment....

Please also add a test that shows that <, = and > now work correctly. You should also now be able to use Timestamps in Map objects, and object equality should now be based on the Timestamp data rather than the address of the Timestamp object. You can test this like this:
Map m = new Map();
m.add(new Timestamp(1,1));
expect(m.has(new Timestamp(1,1)).to.be.true;
Furthermore, can you also add a Changelog entry: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/CHANGELOG.md

Your PR description should also include "Fixes: " which will auto-close the issue on merging.

Implementing valueOf() does not affect the == or === operators when used on Timestamp objects. These operators just test if two objects reference the exact same object and do not test the properties of the objects. By extension, using Timestamp objects as keys in a Map will not change as a result of this PR.

I have updated the PR description as suggested and will update the changelog as well.

…dStart().

This fixes the issue where the minified js contains __PRIVATE_padStart() instead of just padStart().

…hurn.

schmidt-sebastian

Some comment nit.

I also checked with @Feiyang1 and he says to go through API review.

schmidt-sebastian · 2020-02-27T22:57:50Z

packages/firestore/CHANGELOG.md

@@ -5,6 +5,9 @@
  `Query.limitToLast(n: number)` in Firestore 1.7.0 (Firebase 7.3.0) (#2620).
 - [fixed] Fixed an issue where `CollectionReference.add()` would reject
  custom types when using `withConverter()` (#2606).
+- [feature] Implemented Timestamp.valueOf() so that Timestamp objects can be


Nit: Enclose method name in backticks.

You also want to move this to the top, since we are using reverse chronological order.

schmidt-sebastian · 2020-02-27T22:59:12Z

packages/firestore/src/api/timestamp.ts

@@ -47,7 +49,7 @@ export class Timestamp {
      );
    }
    // Midnight at the beginning of 1/1/1 is the earliest Firestore supports.


Please move this comment to line 21, maybe worded a bit differently ("The beginning of 1/1/1")

schmidt-sebastian · 2020-02-27T23:00:34Z

packages/firestore/src/api/timestamp.ts

+  // to have a non-negative value and both <seconds> and <nanoseconds> are left-padded with zeroes
+  // to be a consistent length. Strings with this format then have a lexiographical ordering that
+  // matches the relative ordering of the Timestamp objects whose valueOf() method returned them.
+  // The <seconds> translation is done to avoid having a leading negative sign (i.e. a leading '-'


s/relative ordering of the Timestamp objects whose valueOf() method returned them/expected ordering.

schmidt-sebastian · 2020-02-27T23:00:57Z

packages/firestore/src/api/timestamp.ts

@@ -92,4 +94,21 @@ export class Timestamp {
      ')'
    );
  }
+
+  // Overriding valueOf() allows Timestamp objects to be compared in JavaScript using the


Nit: The top part should be a JSDoc-style method comment.

schmidt-sebastian · 2020-02-27T23:01:49Z

packages/firestore/src/api/timestamp.ts

+  // arithmetic comparison operators, such as < and >.
+  // https://github.com/firebase/firebase-js-sdk/issues/2632
+  //
+  // This method returns a string of the form <seconds>.<nanoseconds> where <seconds> is translated


Can you move this comment into the method? You are describing implementation details. The consumer of the method should only now that the valueOf() return type should sort naturally.

schmidt-sebastian · 2020-02-28T18:26:21Z

packages/firestore/src/api/timestamp.ts

+    // having a leading negative sign (i.e. a leading '-' character) in its string representation,
+    // which would affect its lexiographical ordering.
+    const adjustedSeconds = this.seconds - MIN_SECONDS;
+    const formattedSeconds = String(adjustedSeconds).padStart(12, '0');


Maybe add comment that the year 10,000 as 12 digits.

schmidt-sebastian · 2020-02-28T18:27:16Z

packages/firestore/src/api/timestamp.ts

@@ -92,4 +94,25 @@ export class Timestamp {
      ')'
    );
  }
+
+  /**


How about:

/** * Converts this object to a primitive string, which allows Timestamp objects to be compared * using the `>`, `<=, `>=` and `>` operators. */

Rationale:

You are providing the override. A user doesn't need to do further overriding.

We generally don't link to GitHub issues in top-level method comments. Feel free to move the link to the implementation if you think it provides value.

We are in JavaScript land, so I dropped the explicit mention of the language name.

You also need to copy this API to https://github.com/firebase/firebase-js-sdk/blob/master/packages/firebase/index.d.ts and https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore-types/index.d.ts

Feiyang1

LGTM for tsconfig.base.json and firebase/index.d.ts.

dconeybe added 3 commits February 21, 2020 15:28

Add unit tests for the argument validation logic in the Timestamp class.

6f21f52

Refactor Timestamp constructor to use private static readonly variabl…

fbe0543

…es for the bounds of seconds and nanoseconds.

Implement Timestamp.valueOf().

53894ae

This enables comparison of Timestamp objects using the arithmetic comparison operators, such as < and >. #2632

dconeybe assigned dconeybe, schmidt-sebastian and wu-hui and unassigned dconeybe Feb 21, 2020

schmidt-sebastian suggested changes Feb 21, 2020

View reviewed changes

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian and wu-hui Feb 21, 2020

dconeybe added 3 commits February 26, 2020 13:45

Address code review feedback round #1

8176a17

Fix string creation and remove duplicate tests.

d969a56

Add es2017.string to tsconfig.base.json, to enable usage of String.pa…

e0c96d3

…dStart().

dconeybe requested review from Feiyang1, hiranya911 and hsubox76 as code owners February 27, 2020 15:51

dconeybe added 2 commits February 27, 2020 13:11

Add lib.es2017.string.d.ts to packages/firestore/externs.json.

40223a0

This fixes the issue where the minified js contains __PRIVATE_padStart() instead of just padStart().

Reverted some changes to the Timestamp constructor to minimize code c…

4eda2fa

…hurn.

dconeybe assigned schmidt-sebastian and unassigned dconeybe Feb 27, 2020

dconeybe requested review from schmidt-sebastian and wilhuff February 27, 2020 19:46

schmidt-sebastian reviewed Feb 27, 2020

View reviewed changes

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian Feb 27, 2020

dconeybe added 2 commits February 28, 2020 11:00

Address code review feedback round #2

7babfc7

Fix single-character JsDoc typo.

035d326

dconeybe assigned schmidt-sebastian and unassigned dconeybe Feb 28, 2020

schmidt-sebastian suggested changes Feb 28, 2020

View reviewed changes

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian Feb 28, 2020

Address code review feedback round #3

5ad1d24

dconeybe requested a review from egilmorez as a code owner February 28, 2020 19:18

dconeybe assigned schmidt-sebastian and unassigned dconeybe Feb 28, 2020

dconeybe requested a review from schmidt-sebastian February 28, 2020 19:47

schmidt-sebastian approved these changes Feb 28, 2020

View reviewed changes

schmidt-sebastian removed their assignment Feb 28, 2020

dconeybe assigned dconeybe and Feiyang1 and unassigned dconeybe Feb 28, 2020

Feiyang1 approved these changes Feb 28, 2020

View reviewed changes

dconeybe mentioned this pull request Feb 28, 2020

FR: Add valueOf() to Timestamp instances #2632

Closed

dconeybe assigned egilmorez and hsubox76 and unassigned Feiyang1, egilmorez and hsubox76 Feb 28, 2020

hsubox76 approved these changes Mar 2, 2020

View reviewed changes

dconeybe merged commit f24ddc4 into master Mar 2, 2020

dconeybe deleted the dconeybe/TimestampValueOf branch March 2, 2020 18:53

firebase locked and limited conversation to collaborators Apr 2, 2020

@@ @@ -92,4 +94,25 @@ export class Timestamp { @@
                     ')'
                   );
                 }
+                /**

Implement Timestamp.valueOf(). #2662

Implement Timestamp.valueOf(). #2662

Uh oh!

Conversation

dconeybe commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Feb 21, 2020

Uh oh!

dconeybe commented Feb 26, 2020

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Feiyang1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dconeybe commented Feb 21, 2020 •

edited

Loading

schmidt-sebastian left a comment •

edited

Loading

schmidt-sebastian Feb 21, 2020 •

edited

Loading