-
Notifications
You must be signed in to change notification settings - Fork 156
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: Remove Temporal.Calendar
class and protocol
#2854
Comments
While working on the test262 tests for this, I realized that if we only have builtin calendars, it would be possible to remove the 4th "reference ISO day" argument from the PlainYearMonth constructor, and the 4th "reference ISO year" argument from the PlainMonthDay constructors. So These 4th arguments have always been a bit weird, as we have to basically say in the documentation "Don't use this argument, it's only for custom calendar implementations." Removing it would be a minor win for learnability/comprehensibility. On the other hand, you'd have a piece of the data model (reference day / year) which was not directly settable in the constructor. (You'd still be able to read the value using Any thoughts on this? I'm fairly neutral on it. |
How would Hijri adjustment days work with and without custom calendars? I am curious as I will eventually be dealing with that use case. As for the second use case. It's very important when dealing with rrules and calendar information. But I am curious to see what the proposed solution would be to have a better understanding of the pros and cons as well as the limitations. As long as there is a garentee that the functionality would still be achievable it might not matter much at the end. With the current state of the polyfill I managed to build an rrule library (not open sourced yet) using Temporal and custom calendars allows me to support custom week starts and non-Gregorian recurrence rules. My library is currently fully compliant with RFC 5545 and RFC 7529. I am sharing this to give the champions an idea of how custom calendars would be used. |
@ptomato If, in a later proposal, we go with the "eagerly calculate properties" (#2852) replacement for the current custom calendar API, then could the ISO reference day or year be supplied by the calendar inside the PMD/PYM constructor execution? If so, then we'd never need to add those constructor arguments back. |
@khawarizmus I'm fairly confident that even if it's not possible to do Hijri adjustment days in the form of a pluggable calendar that you can pass to the PlainDate constructor, it'll still be possible to achieve the same functionality. I'd like to learn more about this use case. The way I imagine it'd work would be that you'd create a class something like class HijriAdjustmentPlainDate {
#underlyingPlainDate;
} with a Temporal.PlainDate used internally to implement all the operations while applying the adjustment days (which I don't know the data model of, so that's what I'd like to learn more about.) I'd also like to learn more about the other use case. If it would be possible to share your library (even privately) that would be really helpful. |
@justingrant Unfortunately no. (And the more I think about this, the more I think we should probably not remove the 4th argument; it's weird either way and it's probably not worth it to swap one kind of weirdness for the other.) It's because the constructor arguments are ISO. Taking PlainYearMonth as an example, if you did |
Shu-yu, Shane and I have an internal meeting to sync our position. I agree with Shu-yu that as long as we still allow the implementation to support the calendar the Intl.DateTimeFormat supported via a build-in calendar route, I am fine we strip off the 'user defined" calendar support from Temporal. |
Meeting 2024-05-23: At the upcoming June 2024 TC39 plenary meeting, we'll propose removing the Also, removing this expensive design now enables a follow-up proposal to implement custom calendars in a simpler and more space-efficient way, for example @FrankYFTang's idea in #2852 to compute properties in a single user-code call or to handle object construction using fewer methods like what's proposed in #2851. These ideas are very promising, but changes that large are well beyond the scope of Temporal V1. |
@ptomato For the first use case, the available CLDR Hijri calendars are typically used for civil and administrative purposes, aiding in long-term planning and day-to-day scheduling. However, many Muslim countries practice moon sighting for religious purposes, which can cause Hijri dates to shift by a day or two (up to a maximum of three days if I am not mistaken). Since moon sighting is region-based, there is no one-size-fits-all solution. Therefore, it's essential to accommodate this variability when building systems that handle Hijri calendars. There are two ways to address this:
A custom calendar would help with these adjustments from the perspective of centralising the logic where you would initiate that calendar once and use it with all the Temporal objects (PlainTime, PlainDateTime, ZonedDateTime) across the code base. otherwise you would be using As for the second use case. I have given you (write) access to the library I am working on and I will reach out to chat about it a bit. but you can check the rrule part of the code and see the tests we have. It is worth noting that the custom calendar logic used actually comes from the week-dates library that I open-sourced and there I have a |
trying to better understand the impact of this change - With this change, implementing a custom calendar like we did for Nepali calendar here will not be possible once Temporal.Calendar is removed, right? Will there be a different way to implement such a calendar? As an aside - how do implementers decide which calendars to support typically (and why Nepali for example is not one of them)? |
@kabaros It won't be possible to create a PlainDate object that transparently uses the Nepali calendar as if it was a built-in calendar, but it will still be possible to achieve the same result; just a bit less convenient because you will have to define a Nepali-specific PlainDate class that extends PlainDate. It's on my to-do list to prepare a proof of concept for this. I haven't gotten around to it yet, but the principle would be similar to the custom time zone example I'm working on in PR #2894. Longer-term, your best course of action (regardless of whether Temporal would support custom calendar objects or not) is to work to get the Nepali calendar supported in the Common Locale Data Repository (CLDR). JS implementations support exactly the calendars that CLDR supports. If the calendar is supported in CLDR, you'll also get proper localization, which would not have been possible with Temporal's custom calendars. I don't know why the Nepali calendar is not included in CLDR, but I'm sure it's not from lack of will. More likely they have just never been approached by any experts on the Nepali calendar and would love to work with you on it. @sffc can tell you more about this process. (This is also what we advised @khawarizmus about the week numbering for Hijri calendars.) |
There are CLDR tracking issues to add additional south Asian calendars:
You could add one for Nepalese. These types of contributions would be welcome into CLDR/Unicode, and by doing so, not only do you get the calendar in Temporal, you also get it in Intl.DateTimeFormat, and you also get it everywhere else CLDR is used, including Android, iOS, Windows, and most other platforms. |
This is a very large change, as it not only removes Temporal.Calendar and Temporal.TimeZone, but also tries to eliminate any extra complexity due to no longer having to deal with user code calls for calendar and time zone calculations. Some of the things that are removed or simplified include: - No more Calendar Method Records and Time Zone Method Records - In many places, no need to pass around the user's original options bag - In many places, no need to pass around the user's original PlainDate or Instant; use epoch nanoseconds, ISO Date Records, and ISO Date-Time Records instead - No more copying the own properties of options bags - Most of the calendar and time zone operations are now infallible - The set of extra calendar fields that used to be returned by Temporal.Calendar.prototype.fields() is now static; so no need to have the complicated PrepareTemporalFields operation that returns a null- prototype object with own data properties that correspond to arbitrary user fields. Dates in calendar space can be represented by a Calendar Fields Record with known fields. - Much of the special-casing to avoid user calls that was added in #2519 and similar PRs is now unobservable and is removed. Closes: #2836 Closes: #2853 Closes: #2854
This is a very large change, as it not only removes Temporal.Calendar and Temporal.TimeZone, but also tries to eliminate any extra complexity due to no longer having to deal with user code calls for calendar and time zone calculations. Some of the things that are removed or simplified include: - No more Calendar Method Records and Time Zone Method Records - In many places, no need to pass around the user's original options bag - In many places, no need to pass around the user's original PlainDate or Instant; use epoch nanoseconds, ISO Date Records, and ISO Date-Time Records instead - No more copying the own properties of options bags - Most of the calendar and time zone operations are now infallible - The set of extra calendar fields that used to be returned by Temporal.Calendar.prototype.fields() is now static; so no need to have the complicated PrepareTemporalFields operation that returns a null- prototype object with own data properties that correspond to arbitrary user fields. Dates in calendar space can be represented by a Calendar Fields Record with known fields. - Much of the special-casing to avoid user calls that was added in #2519 and similar PRs is now unobservable and is removed. Closes: #2836 Closes: #2853 Closes: #2854
This issue was split from #2826. This issue is solely focused on removing the
Calendar
class and protocol. Please keep discussions on-topic.Conclusion in the champions meeting of 2024-04-18. We've been asked to reduce the size and complexity of the proposal. The callable hooks in the time zone and calendar protocols are the part that implementations have been most uncomfortable with. They also seem to be where we get the biggest reduction in complexity for the least loss of use cases. The TimeZone and Calendar classes themselves make less sense to keep if the protocols are gone. So our conclusion is to remove them. This issue is focused on calendars only. A separate issue #2853 discusses time zones.
Motivations for removing custom calendars
Use cases that disappear, and their replacements
Work around incomplete/outdated support in CLDR calendars
For example:
Replacement: Previously, you could implement a custom calendar by creating an object that implemented the protocol, and monkeypatching or wrapping some of Temporal so that it would deserialize dates with your custom calendar ID into a Temporal object with the correct protocol object. You would now have to monkeypatch or wrap more of Temporal. As part of moving this issue forward, we'll create a proof of concept for doing this, to make sure that it remains possible.
Scope of issue
calendar
property of object returned by PlainDate, PlainDateTime, PlainYearMonth, PlainMonthDay, ZonedDateTime's getISOFields() methods can only be a stringThe text was updated successfully, but these errors were encountered: