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

FR: Add valueOf() to Timestamp instances #2632

Closed
davidwkeith opened this issue Feb 14, 2020 · 6 comments
Closed

FR: Add valueOf() to Timestamp instances #2632

davidwkeith opened this issue Feb 14, 2020 · 6 comments

Comments

@davidwkeith
Copy link

@davidwkeith davidwkeith commented Feb 14, 2020

[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

This comment has been minimized.

Copy link
Contributor

@wu-hui wu-hui commented Feb 14, 2020

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

This comment has been minimized.

Copy link
Author

@davidwkeith davidwkeith commented Feb 14, 2020

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

This comment has been minimized.

Copy link
Contributor

@wu-hui wu-hui commented Feb 18, 2020

Thanks, I think this will work.

We will close this issue when the change is merged.

@wu-hui

This comment has been minimized.

Copy link
Contributor

@wu-hui wu-hui commented Feb 19, 2020

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

dconeybe added a commit that referenced this issue Feb 21, 2020
This enables comparison of Timestamp objects using the arithmetic comparison operators, such as < and >.

#2632
dconeybe added a commit that referenced this issue Feb 21, 2020
This enables comparison of Timestamp objects using the arithmetic comparison operators, such as < and >.

#2632
@dconeybe

This comment has been minimized.

Copy link
Contributor

@dconeybe dconeybe commented Feb 28, 2020

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 added a commit to googleapis/nodejs-firestore that referenced this issue Feb 28, 2020
This enables Timestamp objects to be compared for relative ordering using the arithmetic comparison operators (i.e. <, <=, >, and >=).

Fixes firebase/firebase-js-sdk#2632
@dconeybe

This comment has been minimized.

Copy link
Contributor

@dconeybe dconeybe commented Feb 28, 2020

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.
dconeybe added a commit that referenced this issue Mar 2, 2020
This enables comparison of Timestamp objects using the arithmetic comparison operators, such as < and >.

Fixes: #2632
@firebase firebase locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.