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

size-suggestion: Further reduce methods of Temporal.Now? #2862

Closed
justingrant opened this issue May 24, 2024 · 13 comments
Closed

size-suggestion: Further reduce methods of Temporal.Now? #2862

justingrant opened this issue May 24, 2024 · 13 comments
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns

Comments

@justingrant
Copy link
Collaborator

In #2846, we're proposing to shrink Temporal.Now to fewer methods:

  • instant()
  • zonedDateTimeISO()
  • plainDateTimeISO()
  • plainDateISO()
  • plainTimeISO()
  • timeZoneId()

In #2846 (comment), @littledan suggested an even further simplification: replace the Temporal.Now object with a single Temporal.now() function that returns a Temporal.ZonedDateTime instance. All the methods of Temporal.Now would be available via a single method or getter call from the resulting ZDT object:

  • Temporal.Now.instant() => Temporal.now().toInstant()
  • Temporal.Now.zonedDateTimeISO() => Temporal.now()
  • Temporal.Now.plainDateTimeISO() => Temporal.now().toPlainDateTime()
  • Temporal.Now.plainDateISO() => Temporal.now().toPlainDate()
  • Temporal.Now.plainTimeISO() => Temporal.now().toPlainTime()
  • Temporal.Now.timeZoneId() => Temporal.now().timeZoneId

Doing this would save 5 methods in code size and would simplify developer experience.

Some possible downsides:

  • It'd make getting the current Instant considerably more expensive, because today Temporal.Now.instant() doesn't require calling SystemTimeZoneIdentifier, which is an ICU call and I assume it's not super-cheap. (Although it may be cacheable so this cost might be easy to optimize?)
  • It'd requires one extra object to be created for every non-ZDT result. ZDT is a fairly inexpensive type to create: its data model is only three values: an 84-bit epoch-nanoseconds value, a ~14-bit built-in time zone ID, and a 4-bit built-in calendar ID. But object creation ain't free.
  • Temporal.now() would have an ISO calendar, so if in the future we wanted to expose the system default calendar via Temporal, we'd need to add a new method, e.g. Temporal.nowWithSystemCalendar() or, at the very least, Temporal.systemCalendar() that could be passed to withCalendar.

Note that there are intermediate possible solutions too, for example, leaving Temporal.Now with only two methods: instant() (for performance) and zonedDateTimeISO() (or maybe zonedDateTime() depending on the renaming discussion in #2846 (comment)).

@justingrant justingrant changed the title size-suggestion: can we further reduce methods of Temporal.Now? size-suggestion: Further reduce methods of Temporal.Now? May 24, 2024
@justingrant justingrant added the size-suggestion Suggestions to shrink size of Temporal to address implementer concerns label May 24, 2024
@sffc
Copy link
Collaborator

sffc commented May 24, 2024

  • Temporal.now() would have an ISO calendar, so if in the future we wanted to expose the system default calendar via Temporal, we'd need to add a new method, e.g. Temporal.nowWithSystemCalendar() or, at the very least, Temporal.systemCalendar() that could be passed to withCalendar.

I'm concerned about Temporal.now() using the ISO calendar because it's not clear whether it should default to "iso8601" or the locale's calendar (new Intl.DateTimeFormat().resolvedOptions().calendar). The champions had previously agreed not to use the locale's calendar implicitly, but also not to use the ISO calendar implicitly. To be sure, I have set up time to discuss this with my team, but for now I would assume that the previous position still stands.

Something like Temporal.nowISO() would alleviate that issue.

Note that there are intermediate possible solutions too, for example, leaving Temporal.Now with only two methods: instant() (for performance) and zonedDateTimeISO() (or maybe zonedDateTime() depending on the renaming discussion in #2846 (comment)).

Two methods with instant() and zonedDateTimeISO() SGTM

@justingrant
Copy link
Collaborator Author

it's not clear whether it should default to "iso8601" or the locale's calendar

Could we just put "system calendar" in the method name to make it super-clear what's going on? Something like:Temporal.nowWithSystemCalendar() or Temporal.nowSystemCalendar() that's easy to discover via IDE autocomplete next-door to the ISO method? Users could learn by omission that the other method like Temporal.now() or Temporal.nowISO() does NOT use the system calendar. We don't need both to be explicit as long as one is.

This is analogous to why I'm arguing to retain Date.p.toZonedDateTime, because having "Zoned" show up in the IDE's autocomplete list for PlainDate will help the user infer that Date.p.toPlainDateTime is "not zoned". In both cases, we'd use the more recognizable term ("system calendar" or "zoned") to help users understand the other variant.

@littledan
Copy link
Member

littledan commented May 24, 2024

OK we probably at least want Temporal.Now.instant(), but I do wonder what kinds of use cases we have for Temporal.Now.plainDateTimeISO(). Was it just introduced for orthogonality, or maybe because we didn't have ZDT when it was introduced?

@justingrant
Copy link
Collaborator Author

I think it'd be OK to remove plainDateTimeISO().

@ptomato
Copy link
Collaborator

ptomato commented May 24, 2024

It is certainly true that we didn't have ZDT when plainDateTimeISO() was introduced.

@justingrant
Copy link
Collaborator Author

I'm hearing a rough consensus above for removing plainDateTimeISO. Should we present this in Helsinki?

We could do more but it sounds like we'd need more discussion about those, but that shouldn't stop us from moving fwd on the non-contentious removal?

@justingrant
Copy link
Collaborator Author

justingrant commented May 29, 2024

Here's a few options we can discuss (at this week's champions meeting) for what additional functions could be removed from Temporal.Now.

Option 1

  • Remove plainDateTimeISO(), no other changes. Removed method is replaced by Temporal.Now.zonedDateTime().toPlainDateTime()

Option 2

  • Remove plainDateTimeISO(), plainDateISO(), plainTimeISO(), and timeZoneId(). Leave only zonedDateTimeISO() and instant(). Removed methods are replaced by:
    • plainDateTimeISO() => zonedDateTimeISO().toPlainDateTime()
    • plainDateISO() => zonedDateTimeISO().toPlainDate()
    • plainTimeISO() => zonedDateTimeISO().toPlainTime()
    • timeZoneId() => zonedDateTimeISO().timeZoneId

Option 3

  • Remove Now object completely; replace with functions
  • Temporal.now() or Temporal.nowISO() - returns ZDT in ISO calendar
  • Temporal.nowInstant() - returns current Instant
  • (future proposal) Temporal.nowWithSystemCalendar() - returns ZDT in system calendar

Personally I like option 2 and option 1 about equally. Option 2 shrinks more functions, while option 1 makes PlainTime, PlainDate, and timezone IDs more discoverable for n00bs. Both are IMO useful goals. If we choose option 2, we can always extend Now in a later proposal in response to feedback.

I assume that Option 3 would be harder to get approval, both because it's a larger change and also because there's no umbrella object to disable for hosts that want to disallow code to access the host environment, which was AFAIK the main reason we have a Now object and not Temporal.ZonedDateTime.now(), Temporal.Instant.now(), etc.

Regardless of the options chosen, I think it'd be better to remove the ISO suffix for the reasons outlined here.

@gibson042
Copy link
Collaborator

I assume that Option 3 would be harder to get approval, both because it's a larger change and also because there's no umbrella object to disable for hosts that want to disallow code to access the host environment, which was AFAIK the main reason we have a Now object and not Temporal.ZonedDateTime.now(), Temporal.Instant.now(), etc.

To be more specific, the principle there was separation—if the capability to read a system clock were on the various classes, then they could not be safely shared in general, but not sharing them would also come at the expense of their purely computational functionality. Temporal.Now is the cleanest way to partition off that capability, although it would still be feasible with Temporal.now$Type functions.

@littledan
Copy link
Member

littledan commented May 29, 2024

Option 2 and 3 both sound good to me.

@SimenB
Copy link
Contributor

SimenB commented May 29, 2024

(fwiw, a shared Now namespace makes it easier for mocking (e.g. Jest, @sinonjs/fake-timers etc., ref sinonjs/fake-timers#335 (comment) ) to ensure that they cover all ways of getting a system dependant value. So removing would make that more whackamole)

@justingrant
Copy link
Collaborator Author

Meeting 2025-05-30: No consensus to remove more functions.

@sffc
Copy link
Collaborator

sffc commented Jun 2, 2024

The TC39 Agenda has the following code snippet that would break if we made this change.

Temporal.ZonedDateTime.from('2024-06-11T10:00[Europe/Helsinki]')
  .withTimeZone(Temporal.Now.timeZoneId()) // your time zone
  .toLocaleString()

@ptomato
Copy link
Collaborator

ptomato commented Jun 13, 2024

After the slate of removals that was approved on 2024-06-12, we are not going to pursue this unless it is really an obstacle for implementations to ship.

@ptomato ptomato closed this as completed Jun 13, 2024
@ptomato ptomato closed this as not planned Won't fix, can't repro, duplicate, stale Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns
Projects
None yet
Development

No branches or pull requests

6 participants