-
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
Remove time zone and calendar protocols and classes #2826
Comments
Datetimes with custom time zones are a pretty major part of working with iCalendar (RFC 5545; https://datatracker.ietf.org/doc/html/rfc5545), since the spec has no provision for reference to a common time zone database. It would be very helpful to get a picture of what this might look like with the revised scope of this proposal. |
This is a very heavy change considering that we are this far into the proposal. Do i understand that there is a guarantee that use cases like this https://week-dates.netlify.app/calendars/iso-extended.html or this https://week-dates.netlify.app/calendars/hijri-week-calendars.html will still be possible with some additional implementation details? Also for the proof of concept. I would be happy to collaborate and make it work in https://week-dates.netlify.app for both aforementioned use cases |
@leftmostcat Our resident RFC 5545 expert @justingrant may be able to give you a better answer about VTIMEZONEs. For TZID, It looks like it's recommended but not required to use identifiers from TZDB so my guess is that for most usages the builtin time zones would be sufficient; hopefully the usages where they weren't, would be covered by the proof of concept I intend to create.
@khawarizmus Yes, the intention is that you'll still be able to do things like this, but you'll have to either do it by not overriding built-in Temporal APIs, or monkeypatching more of Temporal if you want the custom calendar to appear built-in. I hope to illustrate more with the proof of concept. I understand it's a heavy change but I don't think we'll be able to move the proposal forward without it. |
We understand that removing custom time zones from Temporal V1 will create challenges for any application that needs to take a datetime+VTIMEZONE from an iCalendar message and turn that into a But at this point it's not clear we have another option. We need to remove this functionality so that we can ship the rest of Temporal. Here's why:
So to summarize, it seems very, very likely at this point that the current custom time zone implementation will be removed. But it's also likely that there will be lots of demand for a built-in custom timezone solution in a future proposal. And it seems likely that any future solution will use a data format (ideally one based on an existing standard, of which JSCalendar seems closest in spirit to ECMAScript's design) to define a time zone instead of custom userland code. I know this isn't a great answer for folks who have been interested in making custom time zones, but it does seem like the only realistic path forward so we can get Temporal finally shipped into browsers. |
I understand the need for a change in the spec. I'm just hoping it's possible to find a way to provide for use cases where tzdb can't be the source of truth. As noted, it's possible to fall back to tzdb in a lot of cases, but there are common clients out there which don't use standard IDs (Microsoft Exchange, notably). I have trouble seeing a way to implement that with the current API minus custom time zones. It would be a more invasive change to the proposal, but something like
|
@leftmostcat You can effectively have OffsetDateTime already, by using ZonedDateTime with a fixed UTC offset, which is also a valid time zone identifier. For example, |
This solution feels problematic, as it means that common operations which are currently part of Building the API around |
In hindsight, since I don't understand the issues in optimization, I recognize that my posts may not be very helpful. Hopefully I can provide some insight into what an iCalendar implementation needs out of time zones: As I mentioned, though IANA tzids are conventional, there are implementations producing non-IANA VTIMEZONE IDs which are in common use. Establishing equivalence to an IANA tzid is non-trivial given that most implementations provide a restricted definition of the time zone, and both time zone definitions and events may include recurrence rules which may recur indefinitely. As such, implementing iCalendar requires a mechanism outside of tzdb. Using Data-driven custom time zones seem like a workable path forward, but will also require some care in specifying. iCalendar offers a lot of flexibility in specifying recurrence rules, and there isn't good data on how they are actually used in VTIMEZONE definitions. It's worth noting that the set of recurrence rules which can produce any occurrences (in our case, transitions from one offset to another) is a strict subset of the set of recurrence rules iCalendar can encode. For example, a recurrence rule can specify "the first Monday of the year, so long as it occurs between the 15th and the 21st of January". Detecting these is non-trivial and they could be a source of inconsistencies and bugs in implementations. (We had an infinite loop bug due to impossible recurrence rules at one point.)
|
This is so large a change that if it’s going to be necessary, i think the proposal needs to drop to stage 2.7. Separately, i don’t think requiring more monkeypatching is a good change - specifically, i think that “not requiring monkey patching” is enough of a use case to completely outweigh the concerns referenced in the OP. |
If that's what the plenary decides is appropriate, then that would be disappointing but so be it; we wouldn't have proposed this drastic of a change if it wasn't clear that implementation in engines just wasn't going to proceed without it. That said, I'm not sure I see what purpose would be served by dropping the proposal to stage 2.7. Stage 3 means the proposal is recommended for implementation. IMO removing this obstacle for engines only strengthens that recommendation, not weakens it. If we make this change and then engines delay implementing it anyway because the proposal is back at stage 2.7, nobody wins.
I'd like to address this later, maybe even in separate thread, after I dig up more of the discussion history around the use case of polyfilling the time zone database. But just to give a short answer for now, nothing requires any monkeypatching at all! However we do expect 'wanting to use my specific version of time zone info' to be a use case, although rare. Time zone info is inherently global data, so the only way to do this is either by polyfilling (i.e. monkeypatching) or by using user-supplied global data somehow. Way back during stage 2, we explored having a global registry for time zone IDs, but that was (probably rightly) shot down. |
The point of stage 2.7 is to mean that it's time to implement, but not to ship. Continuing normative changes mean it's not ready for stage 3. In this case, are you saying that implementations are refusing to implement it without this change? The wording in the OP suggests merely that it's a request to explore reductions. Process-wise, which aspects of this feedback couldn't have been possible prior to implementation? The observability of the protocols was part of the reviewed stage 2 proposal. |
I think it's simply impractical for an implementation to accurately assess how much code they're going to need to implement a proposal of this size without actually going and trying to implement it. That's why we have "implementation" as a separate stage prior to "landed in the spec", and why changes due to feedback from production-grade implementations are expected during stage 3. |
Which of the "motivation" bulletpoints in the OP require actual implementation work to determine? They all seem like purely design considerations (albeit with an implementor mindset applied). |
I can't speak for implementations, but speaking generally, the extent to which complexity and surface area in a proposed design actually translates to complexity and size in implementations can be hard to judge prior to implementing. |
For example it's difficult to determine how many different CalendarDateAdd functions are needed without having an actual implementation:
The three possible cases for
And two possible cases for the return value:
It's difficult for me to identify which cases apply when just looking at the Without user-defined calendars the This pattern repeats for many other abstract operations. In addition to that, user-defined calendars and time zones can create invalid/unexpected results, which can easily lead to spec bugs: #2235, #2335, #2536, #2700, #2779, #2783, #2801, #2824, etc. This has led to clutter the spec with extra checks just to make sure no invariants are violated. |
Here is my counterproposal "to reduce the surface area and complexity of the proposal" without removing any functionality of custom Calendar and TimeZone by removing 70 functions from Temporal spec (a 21.875% reduction in number of functions). How many functions this proposal can reduce comparing to my counter proposal? |
It would be fantastic if the team looks at ways to reduce the size without having to affect these features. And i tend to agree with @ljharb on this:
Also the question remains open
|
…nsition This is being moved to a method on Temporal.ZonedDateTime.prototype. It will not take a Temporal.Instant argument. See: tc39/proposal-temporal#2826
Temporarily replace them with getISOFields().calendar/timeZone just to keep the tests running until we remove Calendar and TimeZone objects altogether. See: tc39/proposal-temporal#2826
…hods TimeZone.p.getNextTransition → ZonedDateTime.p.nextTransition TimeZone.p.getPreviousTransition → ZonedDateTime.p.previousTransition This is one step towards removing Temporal.TimeZone. The functionality of querying UTC offset transition remains, but is moved to ZonedDateTime. See: #2826 Co-Authored-By: Richard Gibson <richard.gibson@gmail.com>
Calendars and time zones will be only strings, not objects. Therefore these methods that return the calendar or time zone as an object are not needed. See: #2826
…nsition This is being moved to a method on Temporal.ZonedDateTime.prototype. It will not take a Temporal.Instant argument. See: tc39/proposal-temporal#2826
Temporarily replace them with getISOFields().calendar/timeZone just to keep the tests running until we remove Calendar and TimeZone objects altogether. See: tc39/proposal-temporal#2826
Temporarily replace them with getISOFields().calendar/timeZone just to keep the tests running until we remove Calendar and TimeZone objects altogether. See: tc39/proposal-temporal#2826
…nsition This is being moved to a method on Temporal.ZonedDateTime.prototype. It will not take a Temporal.Instant argument. See: tc39/proposal-temporal#2826
Temporarily replace them with getISOFields().calendar/timeZone just to keep the tests running until we remove Calendar and TimeZone objects altogether. See: tc39/proposal-temporal#2826
…hods TimeZone.p.getNextTransition → ZonedDateTime.p.nextTransition TimeZone.p.getPreviousTransition → ZonedDateTime.p.previousTransition This is one step towards removing Temporal.TimeZone. The functionality of querying UTC offset transition remains, but is moved to ZonedDateTime. See: #2826 Co-Authored-By: Richard Gibson <richard.gibson@gmail.com>
Calendars and time zones will be only strings, not objects. Therefore these methods that return the calendar or time zone as an object are not needed. See: #2826
…nsition This is being moved to a method on Temporal.ZonedDateTime.prototype. It will not take a Temporal.Instant argument. See: tc39/proposal-temporal#2826
Temporarily replace them with getISOFields().calendar/timeZone just to keep the tests running until we remove Calendar and TimeZone objects altogether. See: tc39/proposal-temporal#2826
…nsition This is being moved to a method on Temporal.ZonedDateTime.prototype. It will not take a Temporal.Instant argument. See: tc39/proposal-temporal#2826
Temporarily replace them with getISOFields().calendar/timeZone just to keep the tests running until we remove Calendar and TimeZone objects altogether. See: tc39/proposal-temporal#2826
…hods TimeZone.p.getNextTransition → ZonedDateTime.p.nextTransition TimeZone.p.getPreviousTransition → ZonedDateTime.p.previousTransition This is one step towards removing Temporal.TimeZone. The functionality of querying UTC offset transition remains, but is moved to ZonedDateTime. See: #2826 Co-Authored-By: Richard Gibson <richard.gibson@gmail.com>
Calendars and time zones will be only strings, not objects. Therefore these methods that return the calendar or time zone as an object are not needed. See: #2826
…hods TimeZone.p.getNextTransition → ZonedDateTime.p.nextTransition TimeZone.p.getPreviousTransition → ZonedDateTime.p.previousTransition This is one step towards removing Temporal.TimeZone. The functionality of querying UTC offset transition remains, but is moved to ZonedDateTime. See: #2826 Co-Authored-By: Richard Gibson <richard.gibson@gmail.com>
Calendars and time zones will be only strings, not objects. Therefore these methods that return the calendar or time zone as an object are not needed. See: #2826
Conclusion in the champions meeting of 2024-04-18. We've been asked to reduce the surface area 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.
Motivations for removing
Temporal.TimeZone
object is heavily reduced, although it doesn’t become completely redundant likeTemporal.Calendar
.Use cases that disappear, and their replacements
Work around incomplete/outdated support in CLDR calendars and TZDB
For example:
Replacement: Previously, you could implement a custom calendar or time zone by creating an object that implemented the protocol, and monkeypatching some of Temporal so that it would deserialize dates with your custom calendar/time zone's ID into a Temporal object with the correct protocol object. You would now have to monkeypatch 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.
Converting directly between Instant and PlainDateTime
Replacement: Go through ZonedDateTime, instead of through TimeZone. e.g.
Determine if two time zone IDs are aliases
Replacement: Use ZonedDateTime.p.equals() with the same instant. (This is more inconvenient, but it's a pretty uncommon operation. If this really needs to be more ergonomic, a dedicated static method can be added in a follow up.)
Look up previous/next UTC offset transition in a time zone
Replacement: We'll add two methods to ZonedDateTime that replace the two methods on TimeZone.
Custom disambiguation behaviour
A niche use case is implementing PlainDateTime-to-Instant conversions with disambiguation behaviours other than the built-in
earlier
,later
,compatible
modes. By request, we have a cookbook recipe for this.Replacement: You can still do it with two conversions, for example:
Scope of issue
calendar
property of object returned by PlainDate, PlainDateTime, PlainYearMonth, PlainMonthDay, ZonedDateTime's getISOFields() methods can only be a stringtimeZone
property of object returned by ZonedDateTime.p.getISOFields() can only be a stringThe text was updated successfully, but these errors were encountered: