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

Change Notification.timestamp to be an unrestricted double instead #56

Closed
wants to merge 1 commit into from

Conversation

beverloo
Copy link
Member

Date.now() returns a value of the Number type, which maps to WebIDL's
unrestricted double. This allows timestamps in the entire range of
Date to be used, rather than just times after the unix epoch.

@beverloo
Copy link
Member Author

Paging @domenic as this relates closely to whatwg/webidl#2.

The primary question I have is what we expect NaN and infinity to do. Also, we'd want this to be compatible with timestamps in other specs, but this is quite a mess right now.

The Web Animation API defines the behavior for NaN, so we could map that to the current time?

Date.now() returns a value of the Number type, which maps to WebIDL's
unrestricted double. This allows timestamps in the entire range of
Date to be used, rather than just times after the unix epoch.
@domenic
Copy link
Member

domenic commented Oct 22, 2015

I tried to give some guidance in this regard at https://w3ctag.github.io/design-principles/#times-and-dates. In general I think all of these cases should be using DOMTimeStamp if they want epoch-compatible time, or DOMHighResTimeStamp if they want high-precision monotonic time.

We might need to change the definition or behavior of those two types though, since right now they do not account for the cases you mention. /cc @heycam

I see four options for NaN/+Infinity/-Infinity:

  1. What JavaScript APIs like Date would do: just leave them as they are. If serialized to a string, use "Invalid Date".
  2. What this PR does: use the current time
  3. What the spec currently does (unsigned long long): use 0, i.e. the epoch.
  4. What nobody seems to be doing yet: throw a RangeError. (Web IDL's [EnforceRange] uses TypeError, which seems less correct, but also acceptable.)

I don't have a strong opinion between these and would rather let people who do make the decision.

But I do feel strongly that everyone should converge on DOMTimeStamp and DOMHighResTimeStamp, so we can just update definitions in one place and not have to deal with this every time. Maybe after we get this straightened out someone can go on a pull request spree across all the repos you mention.

@heycam
Copy link

heycam commented Oct 22, 2015

Yeah, if we want DOMTimeStamp to do something specific with NaN and +/-Infinity, we'll probably need to turn it into a real type rather than just a typedef for unsigned long long. DOMHighResTimeStamp also doesn't have unrestricted in it so throws for NaN and +/-Infinity.

Not sure I have a strong opinion on what to do with these values either, although I'm not particularly sold on NaN == current time.

@domenic
Copy link
Member

domenic commented Oct 22, 2015

DOMHighResTimeStamp also doesn't have unrestricted in it so throws for NaN and +/-Infinity.

The inconsistency here seems not great, at the very least. (DOMTimeStamp turns them into zero, DOMHighResTimestamp throws.)

@annevk
Copy link
Member

annevk commented Oct 24, 2015

@domenic the problem @beverloo had with DOMTimeStamp was not precision, but reach. It cannot be used to represent events prior to 1970.

@domenic
Copy link
Member

domenic commented Oct 27, 2015

It cannot be used to represent events prior to 1970.

I see. But, this is just an artificial restriction imposed by its definition. We should fix it, at the same time as we decide how to handle NaN and the infinities.

@annevk
Copy link
Member

annevk commented Oct 29, 2015

@beverloo any thoughts?

@annevk
Copy link
Member

annevk commented Jan 12, 2016

@beverloo happy new year #2 ping! 😊

annevk pushed a commit that referenced this pull request Jan 22, 2016
@annevk
Copy link
Member

annevk commented Jan 22, 2016

Fixed by #60.

@annevk annevk closed this Jan 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants