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

Remove time zone and calendar protocols and classes #2826

Closed
ptomato opened this issue Apr 18, 2024 · 43 comments
Closed

Remove time zone and calendar protocols and classes #2826

ptomato opened this issue Apr 18, 2024 · 43 comments
Labels
normative Would be a normative change to the proposal
Milestone

Comments

@ptomato
Copy link
Collaborator

ptomato commented Apr 18, 2024

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

  • Requiring observable calls in a particular sequence makes it difficult for implementations to optimize, unless they maintain separate code paths for built-in vs. user-supplied calendars/time zones.
  • Large increase in complexity of proposal for relatively little gain.
  • This functionality made more sense back when you could monkeypatch Calendar.from() and TimeZone.from() to ‘register’ a custom ID. That ability was removed when the proposal went to stage 3 because the committee will not agree to global state.
  • Without a calendar protocol, there doesn’t seem to be much point in having a Temporal.Calendar object. All the functionality is still easily available through PlainDate methods and properties, (except for Calendar.p.fields() and Calendar.p.mergeFields() which are weird methods that only exist to make it possible to build custom calendars with extra fields.)
  • Without a time zone protocol, the usefulness of a Temporal.TimeZone object is heavily reduced, although it doesn’t become completely redundant like Temporal.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.

// Before
timeZone.getPlainDateTimeFor(instant)
timeZone.getInstantFor(plainDateTime, { disambiguation })

// After
instant.toZonedDateTimeISO(timeZoneID).toPlainDateTime()
plainDateTime.toZonedDateTime(timeZoneID, { disambiguation }).toInstant()

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.)

// Before
Temporal.TimeZone.from('Asia/Kolkata').equals(Temporal.TimeZone.from('Asia/Calcutta'))

// After
zonedDateTime.withTimeZone('Asia/Kolkata').equals(zonedDateTime.withTimeZone('Asia/Calcutta'))

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.

// Before
timeZone.getNextTransition(fromInstant)
timeZone.getPreviousTransition(fromInstant)

// After
const fromZonedDateTime = fromInstant.toZonedDateTimeISO(timeZoneID);
fromZonedDateTime.nextTransition()
fromZonedDateTime.previousTransition()

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:

// Before
arrayOfInstants = timeZone.getPossibleInstantsFor(plainDateTime)

// After
arrayOfInstants = [
  plainDateTime.toZonedDateTime(timeZoneID, { disambiguation: 'earlier' }),
  plainDateTime.toZonedDateTime(timeZoneID, { disambiguation: 'later' }),
]

Scope of issue

  • Remove Temporal.Calendar
  • Remove Temporal.TimeZone
  • User-defined methods are not looked up nor called during calculations
  • Calendar-taking APIs no longer accept objects, only string IDs, and ISO 8601 strings out of which a string ID is determined
  • Time-zone-taking APIs no longer accept objects, only string IDs, and ISO 8601 strings out of which a string ID is determined
  • [[Calendar]] internal slot of PlainDate, PlainDateTime, PlainYearMonth, PlainMonthDay, ZonedDateTime only stores string ID
  • [[TimeZone]] internal slot of ZonedDateTime only stores string ID
  • Remove getCalendar() methods from PlainDate, PlainDateTime, PlainYearMonth, PlainMonthDay, ZonedDateTime prototypes
  • Remove ZonedDateTime.p.getTimeZone()
  • calendar property of object returned by PlainDate, PlainDateTime, PlainYearMonth, PlainMonthDay, ZonedDateTime's getISOFields() methods can only be a string
  • timeZone property of object returned by ZonedDateTime.p.getISOFields() can only be a string
  • Add ZonedDateTime.p.nextTransition() and ZonedDateTime.p.previousTransition()
  • Update TypeScript types to match the changes above.
  • Create proof of concept for how you would polyfill a custom calendar or time zone going forward
@leftmostcat
Copy link

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.

@khawarizmus
Copy link

khawarizmus commented Apr 19, 2024

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

@ptomato
Copy link
Collaborator Author

ptomato commented Apr 22, 2024

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.

@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.

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

@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.

@justingrant
Copy link
Collaborator

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.

@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.

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 Temporal.ZonedDateTime object, especially if the TZID isn't an IANA time zone name. (If it is an IANA zone, you can just use it... and you could validate that the built-in zone matches the VTIMEZONE data before doing that.)

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:

  • Without shrinking the size of this proposal, Google and Apple are reluctant to ship Temporal due to code size and overall complexity. (Especially how Temporal objects can call into user code for calendars and time zones)
  • The current custom time zone protocol was defined when built-in time zones used the same protocol. But built-in time zones were changed to use strings, which made the current protocol inconsistent with built-in zones.
  • Combining both the above concerns, the current consensus is that the best way to define a custom time zone is to use data, not code. There's lots of prior art to choose from for this, including iCalendar's VTIMEZONE or its JSCalendar equivalent, or the data format used in the IANA Time Zone Database itself. TC39 is very careful to avoid shipping the wrong API, even if it means a delay.
  • Earlier changes in the Temporal proposal removed the ability to do string parsing of custom time zone IDs. This meant that support of custom time zones was partial and incomplete. If we do revisit custom time zones then we'd want to solve this too so that we could have round-trip ability including string parsing.

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.

@leftmostcat
Copy link

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 OffsetDateTime instead of ZonedDateTime could potentially offer the same functionality without much change in usability, while potentially also allowing custom time zone definitions.

OffsetDateTime would just store an offset instead of a reference to a time zone. A TimeZone would just need to implement a function mapping an Instant to an offset.

@ptomato
Copy link
Collaborator Author

ptomato commented Apr 29, 2024

@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, Temporal.ZonedDateTime.from({ year, month, day, hour, ..., timeZone: '-04:30' }). Would that be sufficient for the use case you're proposing?

@leftmostcat
Copy link

@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, Temporal.ZonedDateTime.from({ year, month, day, hour, ..., timeZone: '-04:30' }). Would that be sufficient for the use case you're proposing?

This solution feels problematic, as it means that common operations which are currently part of ZonedDateTime need to be reinvented for any consumer which can't use IANA time zones. For example, ZonedDateTime provides the instance method add() for getting a later ZonedDateTime in the same time zone, while accounting for offset changes, etc. With a pseudo-OffsetDateTime, it's now the consumer's responsibility to understand that the time zone on the ZonedDateTime doesn't represent a real time zone, but an offset, and that add() is inappropriate. It is also the consumer's responsibility to implement their own add() functionality which adds the duration and adjusts the offset. In the end, a "correct" implementation of custom time zones requires careful maintenance of private semantics/invariants, which is one of the big problems I encounter using Date.

Building the API around OffsetDateTime means that the semantics of time zones don't have to be abused to get a correct solution. The spec and naming can make it clear that adding a duration to OffsetDateTime will not account for time zone, and those semantics would be universal across implementations. zonedInstance.add(duration) becomes offsetInstance.add(duration).relativeTo(timeZone), where timeZone is anything with an offsetAt(instant) method. The Temporal global can provide something like Temporal.getIanaTimeZone() to get suitable implementations for standard time zones.

@leftmostcat
Copy link

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 ZonedDateTime with added semantics around the "time zone" restricted to representing offset presents many of the same problems as the current Date implementation: the existing API is built around the semantics of a time zone, not an offset, and so an implementer needs to put significant effort into reshaping something against its intended purpose. The built-in APIs will produce incorrect results, so their functionality needs to be recreated. A lot of work also needs to go into making sure that the additional context that the ZonedDateTime isn't really a ZonedDateTime isn't lost. This is error-prone and a lot of wasted effort.

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.)

icu4c's zoneinfo64 data uses a simpler model, with transition times stored as month, day, day of week, and time (with some complexity in how day and day-of-week interact), but due to the lack of data on how real-world implementations use RRULEs in iCalendar, it's hard to say if this is sufficient to encode what implementations produce or how difficult it is to convert from RRULE to zoneinfo64 rule.

@ljharb
Copy link
Member

ljharb commented May 11, 2024

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.

@ptomato
Copy link
Collaborator Author

ptomato commented May 14, 2024

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.

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.

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.

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.

@ljharb
Copy link
Member

ljharb commented May 14, 2024

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.

@bakkot
Copy link
Contributor

bakkot commented May 15, 2024

Process-wise, which aspects of this feedback couldn't have been possible prior to implementation?

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.

@ljharb
Copy link
Member

ljharb commented May 16, 2024

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).

@bakkot
Copy link
Contributor

bakkot commented May 16, 2024

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.

@anba
Copy link
Contributor

anba commented May 16, 2024

For example it's difficult to determine how many different CalendarDateAdd functions are needed without having an actual implementation:

  • The spec always creates a Temporal.Duration object for CalendarDateAdd.
  • Temporal.Duration consist of ten slots which all hold a f64 floating point value.
  • Creating a Temporal.Duration GC object is much more expensive when compared to using a plain C struct on the stack, which also has ten f64 floating point values, so implementation will like to avoid creating unnecessary GC objects.
  • In most cases a more simple C struct with just four int64 values is actually only needed.
  • The spec also always creates a Temporal.PlainDate result object, which is unnecessary when the object will directly consumed

The three possible cases for duration input:

  • CalendarDateAdd is called with a Temporal.Duration GC object.
  • CalendarDateAdd is called with an on-stack C/C++ struct with ten f64 floating point values.
  • CalendarDateAdd is called with an on-stack C/C++ struct with four int64 values.

And two possible cases for the return value:

  • Return a Temporal.PlainDate object which is later passed to user-code.
  • Return a C struct PlainDate with three int32 fields.

It's difficult for me to identify which cases apply when just looking at the CalendarDateAdd callers in the spec text. Only during implementation I started to realise which GC objects are unobservable.

Without user-defined calendars the CalendarDateAdd implementation can likely be simplified to a single function which takes the C/C++ struct with four int64 values as the input and returns the C/C++ struct with three int32 fields.

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.

@FrankYFTang
Copy link
Contributor

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?

@khawarizmus
Copy link

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:

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.

Also the question remains open

In this case, are you saying that implementations are refusing to implement it without this change?

@littledan
Copy link
Member

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.

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.

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.

@ljharb
Copy link
Member

ljharb commented May 20, 2024

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.

@littledan
Copy link
Member

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.

@leftmostcat
Copy link

Given that this change is just a deletion, I'm not convinced that this is destabilizing in a risky way.

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.

@littledan
Copy link
Member

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).

@littledan
Copy link
Member

@leftmostcat Does the usability impact come down to custom tzdb for you, or are there other issues that you care about?

@FrankYFTang
Copy link
Contributor

instant.epochMicroseconds isn't equal to instant.epochNanoseconds / 1000n, though.

If user care about such precision, there are way in the user land to get the equvlant value from instant.epochNanoseconds , right?

@FrankYFTang
Copy link
Contributor

I'll create separate issues for each of Frank's categories of suggestions

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?

@gibson042
Copy link
Collaborator

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?

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).

@FrankYFTang
Copy link
Contributor

FrankYFTang commented May 20, 2024

I disagree that "reduce function count" should be a relevant goal in itself,

The number of "function count" is one key concern from v8. The current Temporal proposal introduce 302 functions (105 getters and 197 functions ).
see https://docs.google.com/document/d/1awbvI4r6BXUtRqE6IbtI_YeAfE4WWXGAu02CjY1cQgg/edit

$ egrep "<h1.*Temporal\." proposal-temporal/spec/*|egrep -v "Properties|Constructor|Object|\. \. \.|range|@@toStringTag"|cut  -d'>' -f2|cut -d'<' -f1|sort -u|egrep  -v "(constructor|prototype)$" | wc -l
302

In comparison, if we run similar counting on the current ECMA262 spec

egrep "<h1" ecma262/spec.html|egrep -v "Properties|Constructor|Object|\. \. \.|range|@@toStringTag"|cut  -d'>' -f2|cut -d'<' -f1|sort -u|egrep  -v "(constructor|prototype)$" |egrep "\(|\.|get "|egrep -v ":|Operator|Functions|@@" |wc -l
460

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

egrep "<h1.*Intl" ecma402/spec/*|egrep -v "Properties|Constructor|Object|\. \. \.|range|@@toStringTag"|cut  -d'>' -f2|cut -d'<' -f1|sort -u|egrep  -v "(constructor|prototype)$" |egrep -v :|wc -l
60

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
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-temporal.cc?q=builtins-temporal.cc&ss=chromium

and therefore linear coorelate to the compiled binary size of "Size of .text + .data + .rodata + .data.rel.ro"
builtins-temporal.cc

mentioned in
#2786

@gibson042
Copy link
Collaborator

gibson042 commented May 20, 2024

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 getTransition(start, direction) rather than getNextTransition(start) and getPreviousTransition(start) would be equally powerful but less discoverable, less comprehensible, and objectively more complex (because the new parameter would require corresponding processing/validation).

Also, the number of function calls are linear coorelate to the code size in v8 https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-temporal.cc?q=builtins-temporal.cc&ss=chromium

and therefore linear coorelate to the compiled binary size of "Size of .text + .data + .rodata + .data.rel.ro" builtins-temporal.cc

mentioned in #2786

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).

@FrankYFTang
Copy link
Contributor

having getTransition(start, direction) rather than getNextTransition(start) and getPreviousTransition(start) would be equally powerful but less discoverable, less comprehensible, and objectively more complex (because the new parameter would require corresponding processing/validation).

I disagree.
Having getNextTransition(start) and getPreviousTransition(start) would be less discoverable, less comprehensible, and objectively more complex because, the list of funciton will be longer to browser through, and the developers have to understand read the documentation of the two function to know their relationship, and more complex because it is actaully conceptually one core concept about the Transition before or after the start but artifically break up by API designer into two separate seciton and now the developer, in order to fully understand are the relationship between these two (no one should assume what will be the same or differences between two funciton of a object, regardless how similar their names are) they have to carefully compare the documentaiton of the two line by line.

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.

new parameter would require corresponding processing/validation

calling new function also would require corresponding processing/validation

@justingrant
Copy link
Collaborator

Please discuss specific suggestions in their respective issues. Thanks!

@FrankYFTang
Copy link
Contributor

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?

@gibson042
Copy link
Collaborator

gibson042 commented May 20, 2024

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 add and subtract methods on the same class than add on one class and negated on another, and easier to comprehend a.since(b) than a.difference(b, { op: "since" })). But not complexity—it is objectively more complex1 to have a single function with a mode parameter that must be processed and validated than to have one function per mode.

calling new function also would require corresponding processing/validation

This is just incorrect. getTransition(start, direction) requires processing of direction that does not exist in getNextTransition(start) and getPreviousTransition(start), whereas all three have identical verification of start.

Footnotes

  1. complex: "composed of two or more parts", "hard to separate, analyze, or solve" as in complect ("intertwine, embrace, interweave"), see also Rich Hickey's "Simple Made Easy".

@leftmostcat
Copy link

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?

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.

@justingrant
Copy link
Collaborator

I'll create separate issues for each of Frank's categories of suggestions

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?

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.

ptomato added a commit to ptomato/test262 that referenced this issue May 26, 2024
…nsition

This is being moved to a method on Temporal.ZonedDateTime.prototype. It
will not take an argument.

See: tc39/proposal-temporal#2826
ptomato added a commit to ptomato/test262 that referenced this issue May 26, 2024
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
ptomato added a commit that referenced this issue May 26, 2024
…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
ptomato added a commit that referenced this issue May 26, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal
Projects
None yet
Development

No branches or pull requests

10 participants