FR: Add valueOf() to Timestamp instances

[davidwkeith]

[REQUIRED] Describe your environment

• Operating System version: macOS 10.15.2 (19C57)
• Browser version: Node v1.21.1
• Firebase SDK version: 7.13.0
• Firebase Product: Firestore

[REQUIRED] Describe the problem

Unable to compare Timestamp instances with native comparators. Javascript calls valueOf() on the object and uses the results of that to do native comparisons.

Steps to reproduce:

Attempt to compare Timestamps directly.

Relevant Code:

firebase = require('firebase-admin')
a = new firebase.firestore.Timestamp(1, 1)
b = new firebase.firestore.Timestamp(2, 2)
console.log(a < b) // false
console.log(b < a) // false

Workaround

Of course you can simply call toMillis() and compare those, but having it auto handled would be better.

[wu-hui]

Hi @davidwkeith

Thanks for you suggestion.

We are a little hesitate to implement valueOf here: Timestamp is two numbers underneath, while valueOf converts it to one number. Although for most cases, such conversion is OK (same way that it is OK to compare them by converting them using toMillis first), it is not an accurate representation of the time stamp.

We might consider exposing its internal _compareTo method though.

[davidwkeith]

I assume that since the library is written in Typescript that it would be OK to use BigInt, if so, the following code would work:

{
  valueOf(): BigInt {
    const secondsStr = this.seconds.toString();
    const nanosecondsStr = this.nanoseconds.toString().padStart(10, '0')
    return BigInt([secondsStr, nanosecondsStr].join(''))
  }
}

[wu-hui]

Thanks, I think this will work.

We will close this issue when the change is merged.

[wu-hui]

Actually, Safari does not support BigInt, we are looking into if we could still represent Timestamp with a number. Will keep you posted.

[dconeybe]

I've opened a ticket for this same issue in the nodejs-firestore sdk: googleapis/nodejs-firestore#944. I will address this issue there too.

[dconeybe]

For posterity, here is the "packet" that I sent to the API Review Council for adding the valueOf() method to the timestamp class.

Background Information

The Timestamp class encodes a date between 1/1/1 and 12/31/9999. This means that Timestamp objects have an intrinsic ordering. It would be convenient for developers if Timestamp objects could be compared directly using the JavaScript arithmetic inequality comparison operators <, <=, >, and >=. This has been requested by an external developer: #2632.

When JavaScript compares two object references using these operators, it first converts them to primitives by invoking the objects' valueOf() methods. If an object does not override the valueOf() method then the implementation inherited from the Object prototype simply returns the object itself. This causes the comparisons to always evaluate to false.

Here is an example adapted from the bug report from the external developer that shows the "unconditionally-false" behavior that is observed when comparing Timestamp objects:

a = new Timestamp(1, 1)
b = new Timestamp(2, 2)
console.log(a < b) // false, which is incorrect
console.log(b < a) // false

Proposed API Change

The proposed API change is to override the valueOf() method in the Timestamp class. The new valueOf() method will return a string of the form <seconds>.<nanoseconds> where both numbers are left-padded with zeroes (e.g. '315537897599.999999999'). When these strings are compared using the arithmetic inequality comparison operators then their relative order will match the relative order of the Timestamp objects whose valueOf() methods returned them. This will allow two Timestamp objects to be compared directly using these operators.

The behavior of the sample code above would become the following:

a = new Timestamp(1, 1)
b = new Timestamp(2, 2)
console.log(a < b) // true, which is correct
console.log(b < a) // false

This proposal has been peer reviewed and is awaiting review from API council before merging into master: #2662.

Alternatives Considered

Alternative 1: Do Nothing

Making no changes to the Timestamp class is a possibility. This would cause Timestamp instances to continue to not be directly comparable via these operators. Developers would need to instead convert to milliseconds using the toMillis() method. This is inconvenient for developers to do, and, as a result, adds risk of human error, which could lead to bugs. It is also inexact because the nanosecond precision is reduced to millisecond precision, causing two Timestamp objects that differ by less than one millisecond to incorrectly compare equal.

Alternative 2: Return number from valueOf()

Implementing valueOf() to return a primitive number would solve this issue. The problem is that the "safe range" of number primitives has only 53 bits of accuracy (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER) and 69 bits is required to store the entire range of Timestamp values. Even if we reduced the nanosecond precision to microsecond precision this would still require 59 bits. So returning a number from valueOf() is not desirable due to the loss of precision which would cause some unequal Timestamp objects to incorrectly compare equal.

Alternative 3: Return bigint from valueOf()

An improvement to returning number would be to return bigint, an arbitrary-precision primitive. This, however, is not viable because bigint is not supported by Edge or Safari browsers.

Pros & Cons

Here are some pros and cons of the proposed API change:

• Pro: Timestamp objects can be compared directly using the arithmetic inequality comparison operators (<, <=, >, and >=).
• Con: The behavior of the == and === operators are unaffected, and will continue to simply compare the object references. This could be counter-intuitive, e.g. if t1<=t2 and t1>=t2 but t1!=t2.
• Con: It is theoretically possible that existing code relies on the current behavior that comparing Timestamp objects using the arithmetic inequality comparison operators unconditionally evaluates to false. This seems unlikely, however, and breaking code that relies on this behavior seems like a reasonable risk/cost.