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

For brevity of LDT=>epochXxx, should LocalDateTime expose convenience properties from Absolute? #932

Closed
justingrant opened this issue Sep 21, 2020 · 7 comments
Labels
documentation Additions to documentation ergonomics non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved zoned-type Part of the effort for a type with timestamp+timezone

Comments

@justingrant
Copy link
Collaborator

LocalDateTime currently exposes a superset of DateTime's API. But logically it also encapsulates an Absolute too. Should it expose epochSeconds, epochMilliseconds, epochMicroseconds,epochNanoseconds convenience properties?

The specific reason would be to address brevity for the common use case where a user has an LDT and wants get a seconds or msecs since epoch. Operations involving epoch and/or legacy Date seem to be the most common brevity complaint that we've heard, so streamlining these use cases may make sense.

ldt.toAbsolute().getEpochMilliseconds(); // current
ldt.toAbsolute().epochMilliseconds; // if we adopt #750
ldt.epochMilliseconds;  // proposed in this issue

Related: #750.

@justingrant justingrant added ergonomics zoned-type Part of the effort for a type with timestamp+timezone labels Sep 21, 2020
@ljharb
Copy link
Member

ljharb commented Sep 21, 2020

Since LocalDateTime encapsulates an Absolute, it seems reasonable for all of Absolute's methods/getters to be present on it.

@ptomato
Copy link
Collaborator

ptomato commented Sep 22, 2020

I agree that whichever form we decide in #750 should be added to LocalDateTime as well.

@mattjohnsonpint
Copy link
Collaborator

I'll dissent here. I have seen way too many cases where people think that timestamps based on the Unix epoch should somehow be localized, rather than based on UTC. For example, in Moment.js:

const m = moment();
console.log(+m);
console.log(+m.utc());
console.log(+m.tz('Asia/Tokyo'));

Those will all output the exact same timestamp, but I've seen too many examples where people expect them to be different. Sometimes they'll even do a .toDate(), and then expect those Date objects to be different.

In other words:

const ldt1 = Temporal.now.localDateTime('America/New_York');
const ldt2 = Temporal.now.localDateTime('Asia/Tokyo');
ldt1.epochMilliseconds === ldt2.epochMilliseconds  //=> true, but may be surprising to some
ldt1.toAbsolute().epochMilliseconds === ldt2.toAbsolute().epochMilliseconds  //=> true, and is not surprising

@justingrant
Copy link
Collaborator Author

Those will all output the exact same timestamp, but I've seen too many examples where people expect them to be different. Sometimes they'll even do a .toDate(), and then expect those Date objects to be different.

ldt1.toAbsolute().epochMilliseconds === ldt2.toAbsolute().epochMilliseconds  //=> true, and is not surprising

Hi @mj1856 - what do you think makes the first case surprising but the second one unsurprising? They seem very similar.

Also BTW, you might want to update your code to use toInstant(). ;-)

@justingrant
Copy link
Collaborator Author

Meeting 2020-10-09: Tentative consensus is to add these properties to ZonedDateTime: epochSeconds, epochMilliseconds, epochMicroseconds, epochNanoseconds. The reasons:

  • It's a very common interop use case to get milliseconds or seconds since epoch.
  • It's good to remove the previous inconsistency where all methods from DateTime were exposed in LocalDateTime while all methods from Instant were not.
  • Interop with legacy Date is atop the list of brevity complaints we've heard from early adopters, and doing this would shave off 13 characters for the use case of creating a legacy Date from a ZonedDateTime. (We decided that offering a toLegacyDate() method on ZDT, although briefer, would be a bad idea because users might not understand that the time zone isn't coming along for the ride. By emitting only a number, it's more obvious that the conversion to legacy Date won't have a time zone attached.)
  • For the case that Matt noted above where callers may not be aware that epochXxx values are the same in every time zone, we thought that this was a valid concern. But we didn't think that this concern outweighed the consistency and brevity benefits for the (presumed) majority of users who would understand that epoch is always the same everywhere. We agreed that the "timezone-neutralness" of epoch should be clearly documented because understanding that epoch is timezone-neutral is so important that (regardless of whether we made this change or not) developers will have a hard time being successful with Temporal if they don't understand this core concept.

@mj1856 - feel free to follow up if you disagree with doing this change.

@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Oct 9, 2020
@ryzokuken
Copy link
Member

If this is already included in the ZDT draft? Can we close this one and use the main ZDT issue to track it?

@ptomato
Copy link
Collaborator

ptomato commented Oct 22, 2020

It is. Let's close it.

@ptomato ptomato closed this as completed Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation ergonomics non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved zoned-type Part of the effort for a type with timestamp+timezone
Projects
None yet
Development

No branches or pull requests

5 participants