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: Consider merging Temporal.Now.xISO() and Temporal.Now.x() #2846

Closed
justingrant opened this issue May 20, 2024 · 20 comments · Fixed by #2895
Closed

size-suggestion: Consider merging Temporal.Now.xISO() and Temporal.Now.x() #2846

justingrant opened this issue May 20, 2024 · 20 comments · Fixed by #2895
Assignees
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns

Comments

@justingrant
Copy link
Collaborator

We could combine three Temporal.Now method pairs plainDate/plainDateISO, plainDateTime/plainDateTimeISO, and zonedDateTime/zonedDateTimeISO. We'd also want to remove the ISO suffix from Temporal.Now.plainTimeISO.

To merge these methods without a huge step backwards in ergonomics, each method would need to have its calendarId parameter be optional with the iso8601 calendar as the default. (This is the same behavior as in from methods.)

This issue is adapted from suggestions made by @FrankYFTang to help reduce the size of Temporal to address concerns from implementers (especially Android and Apple Watch) that Temporal's install size and/or in-memory size is too large.

An even-more-radical simplification could be to make all Temporal.Now methods parameterless, and instead rely on withCalendar() on the result to use a non-ISO calendar, and to rely on other conversion methods to change the time zone of the resulting object. (Choosing a time zone from Temporal.Now is rare.)

To reduce one more method, we could also consider merging or changing Temporal.Instant's prototype methods toZonedDateTimeISO and toZonedDateTime, but that merger would be a bit more challenging because:

  • Converting a Temporal.Instant to a Temporal.ZonedDateTime by adding a time zone is one of the most common operations in Temporal, and it's important not to make its ergonomics worse.
  • There are toZonedDateTime functions on PlainDate and PlainDateTime, so consistency across them is also important.

Some historical context: (copied from tc39/ecma402#891 (comment))

The intent of adding that ISO suffix in a few places in the Temporal API (Instant#toZonedDateTimeISO, and the zonedDateTime, plainDate, plainDateTime, and plainTime functions of Temporal.Now) was to help developers to learn that Temporal supports non-Gregorian calendars by ensuring that they encounter a speed bump of a calendarId parameter whenever they use the more-intuitive ISO-less variants of those APIs.

By requiring developers to look up the method in the docs to understand this calendarId parameter, the hope was that developers would:

  • Learn that Temporal supports (and their code might be used with) many calendars like Chinese, Islamic, or Hebrew.
  • Learn about the ergonomic alternative methods that put ISO at the end of those calls when writing code that only uses the ISO calendar.

This API naming decision was difficult. It was the most contentious argument in Temporal's 7+ year history. The two positions could be summarized as:

  1. Almost all people in the world rely on non-Gregorian calendars for some use cases, for example the dates of holidays like Easter, Lunar New Year, or Ramadan. We want to encourage developers to support all use cases of all people worldwide, and to write code that works for all calendars. Calendar localization should be treated no differently from language or currency localization where every language or currency is treated equally. Therefore, and every API that accepts a date input should also require a calendarId.
  2. Non-Gregorian calendars determine religious holiday dates and support other cases like Japanese government documents, but across the universe of software use cases their use is exceedingly rare. The Gregorian calendar is the primary civic calendar in almost every country on Earth. Even countries like Saudi Arabia or Israel that make heavy use of non-Gregorian calendars, it's common to present Gregorian dates side-by-side with non-Gregorian dates. This ensures that developers worldwide are familiar with the Gregorian calendar. Therefore, to simplify developer ergonomics Temporal should treat the ISO calendar as a default, and should offer intuitive API pairs like Temporal.Instant#toZonedDateTime() for the ISO calendar and Temporal.Instant#toZonedDateTimeInCalendar() for non-ISO calendars.

The current API is a compromise between these two positions: ergonomic, ISO-only APIs are available, but they require learning about and using an ISO suffix.

@justingrant justingrant added the size-suggestion Suggestions to shrink size of Temporal to address implementer concerns label May 20, 2024
@FrankYFTang
Copy link
Contributor

My origional suggestion could be found in https://docs.google.com/document/d/16Rd43XB8z6iLC2c9AfF--unDFg_t476tI1PFe7PbbcM/

@ptomato
Copy link
Collaborator

ptomato commented May 20, 2024

I'm quite positive about this one. (This was the design I preferred in the first place, during those discussions many years ago.)

@sffc
Copy link
Collaborator

sffc commented May 20, 2024

It took a very long time for the champions to agree on these methods. They successfully achieved the goal of making it explicit where a calendar is introduced when creating or converting between Temporal types.

I have no opinion on whether to remove the xISO() functions so long as the other functions retain their explicit required calendar argument. @FrankYFTang's proposal is limited to that.

Going a step further and making the calendar argument optional in one way or another would reopen a can of worms and I can't get behind that without further research.

@justingrant
Copy link
Collaborator Author

justingrant commented May 20, 2024

Adding a required "iso8601" parameter value to every call to most Temporal.Now methods is a massive degradation of usability for one of the most common Temporal use cases. The only way that merging these methods would get the champions' consensus would be if the calendar parameter were optional or removed.

Let's discuss further among the champions.

@littledan
Copy link
Member

Have we considered leaving only one method in place, Temporal.Now.zonedDateTimeISO()? We could even call it, Temporal.now().

@justingrant
Copy link
Collaborator Author

justingrant commented May 24, 2024

Meeting 2024-05-23: @sffc will sync with his team about the proposal below. If they're OK with it, then at the upcoming June 2024 TC39 plenary meeting, we'll propose the following:

To save 4 methods, to simplify Temporal DX, and to avoid bugs we've already seen from having pairs of calendar-required vs. calendar-optional methods, we'll remove the following APIs:

  • Temporal.Now.plainDate( calendar, [timeZone] )
  • Temporal.Now.plainDateTime( calendar, [timeZone] )
  • Temporal.Now.zonedDateTime( calendar, [timeZone] )
  • Temporal.Instant.p.toZonedDateTime( { calendar, timeZone } )

Use cases for these methods will be handled using a two-method chain: the first call to xxxISO(), and a second call to withCalendar where the calendar is explicit and required. Example:

// current
Temporal.Now.zonedDateTime('chinese');
instant.toZonedDateTime({ timeZone: 'Asia/Tokyo', calendar: 'japanese' });

// proposed
Temporal.Now.zonedDateTimeISO().withCalendar('chinese')
instant.toZonedDateTimeISO('Asia/Tokyo').withCalendar('japanese');

The xxxISO() APIs will remain:

  • Temporal.Now.plainDateISO( [timeZone] )
  • Temporal.Now.plainDateTimeISO( [timeZone] )
  • Temporal.Now.zonedDateTimeISO( [timeZone] )
  • Temporal.Now.plainTimeISO( [timeZone] )
  • Temporal.Instant.p.toZonedDateTimeISO( timeZone )

In addition, if his team is OK with the changes above, @sffc will also discuss with his colleagues whether it'd be OK to make an additional change: to remove the ISO suffix from the remaining APIs, because a suffix is no longer needed to differentiate between the API pairs. If this renaming is OK with Shane and his team, then we'll propose it in Helsinki as part of the changes above.

If we removed the ISO suffixes, it'd make these methods more ergonomic and less error prone, and it'd also fix the oddity of Temporal.Now.plainTimeISO() which received the ISO suffix to allow the possibility of future time calendars, although no such time calendars exist in CLDR.

If renaming is not OK-ed, we'll simply propose the function removals.

@sffc
Copy link
Collaborator

sffc commented May 24, 2024

Just checking, but how do you feel about moving the iso to the beginning now that it's no longer needed to distinguish arguments, such as Temporal.Now.isoPlainDate() and Temporal.Instant.p.toISOZoneDateTime()

@sffc
Copy link
Collaborator

sffc commented May 24, 2024

Have we considered leaving only one method in place, Temporal.Now.zonedDateTimeISO()? We could even call it, Temporal.now().

Worth discussing in its own issue, I think.

A risk of Temporal.now() is that it reopens the discussion of whether the calendar in the return value of that function should be "iso8601" or new Intl.DateTimeFormat().resolvedOptions().calendar

@justingrant
Copy link
Collaborator Author

Just checking, but how do you feel about moving the iso to the beginning now that it's no longer needed to distinguish arguments, such as Temporal.Now.isoPlainDate() and Temporal.Instant.p.toISOZoneDateTime()

For IDE autocomplete, the suffix is much better because it puts the important parts up-front, so you can type toP or toZ and get the right method. It's half the characters you need to type. So if we must keep the ISO in the name, then I'd want to see it remain as a suffix like today.

Have we considered leaving only one method in place, Temporal.Now.zonedDateTimeISO()? We could even call it, Temporal.now().

Worth discussing in its own issue, I think.

Agreed, I'll create a new issue for this.

@justingrant
Copy link
Collaborator Author

Have we considered leaving only one method in place, Temporal.Now.zonedDateTimeISO()? We could even call it, Temporal.now().

Worth discussing in its own issue, I think.

Agreed, I'll create a new issue for this.

#2862

@ptomato
Copy link
Collaborator

ptomato commented May 27, 2024

Removal of Instant.p.toZonedDateTime, Now.zonedDateTime, Now.plainDateTime, and Now.plainDate can be seen here.

@FrankYFTang
Copy link
Contributor

I personally think the term "iso" is not very helpful because it is ambiguous in the computer world. For example:

When new engineer find a function name with a Iso stem, depending on what s/he learn before, s/he may assume any of above are related.

@littledan
Copy link
Member

Don't forget ISO/IEC 22275:2018, the ECMAScript standard! Maybe it's just referring to ISO JavaScript, in case you forgot which language you were programming in.

@sffc
Copy link
Collaborator

sffc commented May 28, 2024

I would prefer ISO8601 as the suffix, but we settled on ISO after a lot of back and forth.

@FrankYFTang
Copy link
Contributor

so... what is the reason that we need to have a suffix?

@anba
Copy link
Contributor

anba commented May 28, 2024

I'd guess most JavaScript programmers are familiar with Date.prototype.toISOString, so "ISO" in the context of date-time functions should be clear to most.

@justingrant
Copy link
Collaborator Author

I'd guess most JavaScript programmers are familiar with Date.prototype.toISOString

This is 100% correct. When most JS developers hear "ISO" they'll think of the string format: YYYY-MM-DDTHH:MM:SS±HH:MM or YYYY-MM-DDTHH:MM:SSZ.

so "ISO" in the context of date-time functions should be clear to most.

I think the problem is that developers only know "ISO" as a format, not a calendar. Most developers will NOT know that there is an "ISO 8601 calendar" that is almost the same as Gregorian except for eras and week numbering.

So I don't think that JS devs' previous experience with Date.prototype.toISOString will be helpful in teaching them that an "ISO" or "ISO8601" suffix has anything to do with calendars. It may just as likely confuse them, i.e. "what does a calendar have to do with a string format?"

I think we'll be much more successful evangelizing to developers that if they care about calendars, they should call .withCalendar(cal) on a Temporal object. This API is easy to discover via autocomplete or docs, it's unambiguous, it's universal across Temporal, its behavior is obvious from the name, and it contains a familiar word "calendar" as opposed to a confusing and ambiguous word "ISO".

And I question whether the ISO suffix will be successful. I'd recommend removing it, so we'd end up with usage patterns like this:

zdt1 = Temporal.Now.zonedDateTime()

zdt2 = Temporal.Now.zonedDateTime().withCalendar('chinese')

@FrankYFTang
Copy link
Contributor

I chatted with sffc offline and now I understand why he think it is important to have the suffix- since there are two possible ways if there are no explicit arguement to put in-

  1. the pre-existing system default based on the system locale in (new Intl.DateTimeFormat()).resolvedOptions().calendar (which is 'gregory' for many systems, but could be other values for some other users) , or
  2. "iso8601".

My understanding is the following two lines should always be true

Temporal.Now.zonedDateTime().calendarId == (new Intl.DateTimeFormat()).resolvedOptions().calendar // always true
Temporal.Now.zonedDateTimeISO().calendarId == "iso8601"  // always true

@sffc
Copy link
Collaborator

sffc commented May 30, 2024

In the future, adding Temporal.Now.zonedDateTimeLocale() would be a nice counterpoint to Temporal.Now.zonedDateTimeISO().

@justingrant
Copy link
Collaborator Author

Meeting 2024-05-30: Feedback from Shane's team was that it's OK to remove the non-xxxISO functions. Conclusion: we'll remove the non-xxxISO functions. TBD whether it's OK remove the ISO suffix, but at this point it seems like we won't remove that suffix.

ptomato added a commit to tc39/test262 that referenced this issue Jun 13, 2024
ptomato added a commit that referenced this issue Jun 13, 2024
ptomato added a commit that referenced this issue 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

Successfully merging a pull request may close this issue.

6 participants