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
|
I'd prefer to stick with subsetting rather than revisiting design at this stage. Let's not focus too much on the V8 size details and instead think more generally about “what would reduce the implementation and maintenance burden of the proposal”, or otherwise "general complexity”. Stage 3 means we have a default of leaving everything in, if we don’t get consensus on one of these changes. We don’t need consensus to keep things the way they are; that has already been achieved. I am optimistic that this group can come to conclusions on these issues before the next plenary.
Given that this change is just a deletion, I'm not convinced that this is destabilizing in a risky way. If we did want to demote it, though, it'd be a new use of Stage 2.7. It's supposed to be that promoting from 2.7 to 3 is just acknowledging that complete tests exist. There might not be tests out for review for this change right now, but it sounds like you're asking for some assurance of stability, rather than tests, to gate Stage 3. IMO this is what we have demotion to Stage 2 for. In my opinion, it would make sense to demote this proposal to Stage 2 only if there's prolonged disagreement over how/whether to make these cuts. Once we're able to agree on which cuts to make, this proposal should be ready to ship. |
A large part of the stage 2.7 discussion was that stage 3 means “safe to use in production”, and the new stage was meant to indicate “the design is done” so to me being stage 3 definitely conveys a measure of stability. I agree with you that if major design pieces are being revisited, stage 2 makes more sense, but i think that would risk reopening too much of the proposal to non-implementor-driven change to be valuable. |
Generally, the time to get implementation-driven feedback is during Stage 3. This is why I don't think we need to demote this proposal. But if we did, then I think we could execute well on a "scoped demotion" limited to deletions, followed by a rapid re-promotion--we've done such a time-limited scoped demotion (in all but name) in other cases. I wouldn't be in support of a demotion without a similar scoping. |
As a potential consumer of this API, I disagree with this. It's a deletion, but that deletion has a significant impact on usability of the API for documented use cases. At a glance, the shape of the API that remains has the potential to block addressing those use cases in a reasonable manner going forward. The deletions are, ultimately, a significant design change. It's appropriate to treat them as such and ensure that what remains is still a sound design on its own and that it provides room to design reasonable solutions to the problems which the removed API was intended to address. |
About providing a custom tzdb: I think this is typically best handled at the level of the runtime which JavaScript lives on top of. This will be the most reliable way to get the repaired timezones to be applied in all relevant APIs (not just Temporal but also Date and Intl). |
@leftmostcat Does the usability impact come down to custom tzdb for you, or are there other issues that you care about? |
If user care about such precision, there are way in the user land to get the equvlant value from instant.epochNanoseconds , right? |
How about we also create separate issues to consider removing TimeZone protocol and Calendar protocal separately? It is possible we remove only one of them but not the other, right? Do they need to be remove together? |
I disagree that "reduce function count" should be a relevant goal in itself, especially if the functionality is preserved by increased parameterization (which actually adds complexity in the form of new validation steps and cognitive burden on the part of developers). |
The number of "function count" is one key concern from v8. The current Temporal proposal introduce 302 functions (105 getters and 197 functions ).
In comparison, if we run similar counting on the current ECMA262 spec
notice adding the 302 functions/getters from the Temporal spec will increase the number of functions inside ECMA262 by 66% (302/460= 0.656) In comparison, if we run similar counting on the current ECMA402 spec
So the API surface (number of functions) of the entire ECMA402 is only 20% of the Temporal proposal. In other words, the overhead v8 (or other browser implementation) needs to increase to add the supported function of Temporal is x5 than the entire ECMA402 now. Also, the number of function calls are linear coorelate to the code size in v8 and therefore linear coorelate to the compiled binary size of "Size of .text + .data + .rodata + .data.rel.ro" mentioned in |
I don't disagree about what the function counts are; I disagree that function count is a useful metric. Taking the concrete case of #2850 without loss of generality, having
Also in that same issue is the approach to break such linear correlation: #2786 (comment) (i.e., multiplex internally rather than sanding down the exposed API). |
I disagree. If it is only one funciton, it is very clear. There are no differences except the direction moving forward or backward. It make the developer's life easier since they need to learn one instead of two functions.
calling new function also would require corresponding processing/validation |
Please discuss specific suggestions in their respective issues. Thanks! |
How about we also separate out the 40 Calendar and TimeZone methods into different groups and file separate issues to discuss them indivisually too? |
You can disagree about discoverability and comprehensibility (which are subjective and currently lack aggregate data from developer surveying), although I am very confident in my assessment (e.g., it is easier to discover
This is just incorrect. Footnotes
|
My use case is not a custom tzdb as described. Rather, I need to handle time zone definitions which are contained in user-provided files. Because they are definitions rather than identifiers referencing time zones in a known tzdb, there is no buildtime solution and it is necessary for me or anyone else implementing this standard to handle time zones at runtime. Custom calendars are not an issue that's on my radar at this point. |
This is a good idea, and it will help us keep discussions focused. Thanks Frank for this suggestion. Let's continue discussion in the new issues:
To avoid duplication of new feedback and to make it easier for future readers, I'm going to close this issue in favor of the two new ones. |
…nsition This is being moved to a method on Temporal.ZonedDateTime.prototype. It will not take an 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
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: