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

DO NOT MERGE: Proof-of-concept for ZonedDateTime (Instant + TimeZone + Calendar) #700

Closed
wants to merge 47 commits into from

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Jun 24, 2020

This PR is a proof-of-concept of a Temporal type that combines an Instant, a TimeZone, and a Calendar and exposes this combination with a superset of the DateTime API. This type models an event that did happen, or will happen, from the perspective of a particular place on Earth. It supports use cases involving DST-safe date/time math, time zone conversions, and/or ergonomically retaining the same time zone across multiple operations. This PR is intended to continue discussion from #569 about whether such a type makes sense for Temporal.

UPDATE: this type has been approved and we're working on the final details before building a real PR to merge it into Temporal. The real PR will probably land sometime in October 2020, but if you want to play with the type as-is, check out this branch, run npm i && npm run build, and then locally point a browser to any page in the built documentation like ./docs/index.html. If you open the browser devtools console, then you can run code using ZonedDateTime.

I ran into a few tough problems with this type, e.g. cross-time-zone difference, DST corner cases, future times that were valid when stored but are no longer valid due to time zone rule changes, etc. There's a "Discussion" section below with details about these issues and how they were resolved.

As a placeholder, the type is named LocalDateTime. There are a few significant differences between what I'm proposing and what's previously been discussed for ZonedInstant. For example, this proposal includes a calendar. So for now there's a different placeholder name to avoid ambiguity. We are bikeshedding on a final name in #707. EDIT: the final name is "ZonedDateTime".

I've seen from GitHub history that types like this have been declined a few times before, so I know this is a hard problem to solve. Not sure if this proposal is the solution, but I hope it'll help move the discussion forward.

If it's decided that this kind of type doesn't belong in Temporal, that's OK too. I'd try to package it up into an OSS library.

What's in this PR

  • This writeup, including a review of challenges that came up and how they were resolved.
  • A proof-of-concept implementation in ./polyfill/lib/zoneddatetime.mjs. All methods are implemented, but this isn't intended to be a real implementation e.g. using slots, using internal APIs, supporting monkeypatching, etc. The intent was to see if there were fundamental blocking issues with the design and to encourage discussion. I wrote only enough code for those goals, although I'd be happy to help PR a real implementation later if it's decided that this type belongs in Temporal.
  • ./polyfill/poc.d.ts which is a port of index.d.ts that merges in the this proposal's changes . For just the shape of the new type, see ./polyfill/lib/poc/ZonedDateTime.nocomments.d.ts.
  • A minimal set of tests, mostly focused on DST behavior. Most cases are not tested yet but I'll gradually add more tests and fix bugs that result.
  • A few methods on other Temporal types that convert those other types to a ZonedDateTime instance, and a now.zonedDateTime() method.
  • 15 cookbook samples were edited to use ZonedDateTime:
    • Simple use cases seemed about the same (in difficulty, ergonomics, readability, etc.) with or without ZonedDateTime.
    • More complex use cases--especially those that required math operations and/or round-trip conversions (e.g. Instant->DateTime->Instant) seemed easier with ZonedDateTime.
    • I found a DST bug in one of the more complicated cookbook samples. The cause was a duration-kind mismatch: trying to add an exact duration to a DateTime value. ZonedDateTime may help with this class of bugs. It'd also help if Temporal had strongly-typed durations (Interaction of Duration vs DST (or other TZ offset changes) #559, Options for handling "3 kinds of durations" problem #686).
  • There's also a ./polyfill/lib/poc folder and some changes to ./package.json. You can ignore both of these. They're artifacts of how I'm building the POC by authoring in ./polyfill/lib/poc/ZonedDateTime.ts and using build scripts and transforms to produce ./polyfill/lib/zoneddatetime.mjs and the merged Temporal-wide ./polyfill/poc.d.ts. Tests are handled similarly by transforming ./polyfill/lib/poc/ZonedDateTime.test.ts to ./polyfill/test/zoneddatetime.mjs. A real implementation would be .mjs only.

TODO / Open Issues

Goals

The ZonedDateTime type has the following goals:

  • Represent a particular instant in time in a particular time zone, which matches most real-world use cases because most apps are focused on users doing things at a particular time in a particular place.
  • "Feel like Temporal" - Use similar patterns and naming as the rest of the library.
  • "Feel like DateTime" - Provide a familiar API that is mostly a superset of DateTime with only a handful of mostly-forward-compatible differences that are required to accommodate the presence of a time zone. Porting code from DateTime to ZonedDateTime should be easy.
  • Make it easier to work with Temporal for junior developers who may not understand best practices for working with date/time data and time zones.
  • Built-in support for math (add, subtract, and difference) using any of the three duration kinds: exact durations, dateTime durations, and hybrid (RFC 5545) durations. This type's math methods all have a durationKind parameter that allows the caller to declare the meaning of the duration. This allows one-line-of-code math regardless of duration kind. EDIT 2020-07-31: removed durationKind choice per Champions' consensus.
  • Make it easier to build standards-compliant calendar apps. RFC 5545 and related standards-track work like JSCalendar have opinionated definitions of how durations and time/date math should behave in order to ensure interoperability between calendaring apps, but today these durations are very hard to work with in Temporal. ZonedDateTime has built-in support for RFC 5545 durations.
  • Provide DST-safe behavior by default, even for developers who are unsure of DST best practices. By default, hybrid (RFC 5545-compliant) durations will be used for math methods, which provides the safest DST experience for developers who don't understand DST issues well enough to know which duration kind to pick.
  • Provide opt-in options to handle use cases where defaults won't suffice, including dealing with future times stored before time zone definition changes, or handling DST corner cases.
  • Bidirectionally interoperate with other Temporal types. It should be easy to use ZonedDateTime with other types, especially Date and Time.
  • Only use public Temporal APIs. This goal is irrelevant if this type makes it into Temporal, but I found it helpful to validate that this type could be built on top of Temporal's primitives without "cheating". So this proposal doesn't directly call internal Temporal APIs, legacy Date, Intl, or any other dependencies.

Non-goals

  • NOT replace any other Temporal type. Temporal's primitives are already the right ones. This type builds on those primitives.
  • NOT represent non-existent wall-clock times. Non-existent times like the wall-clock hour skipped by DST each Spring cannot be stored in a ZonedDateTime. These times can be parsed and disambiguated by ZonedDateTime (e.g. to handle the case where DST rules change after an ISO string is persisted), but if a ZonedDateTime instance is returned to the caller then it's guaranteed to be a real moment in time in a real time zone. Developers who want to work with invalid date/time data can drop down to a lower level of abstraction.
  • NOT assume that defaults that work for all use cases. Developers using relatively uncommon use cases should expect to have to opt in to the desired behavior. Usually this is as simple as specifying a different durationKind option, but some unusual cases (e.g. switching a ZonedDateTime to another time zone while keeping clock time constant) may require a little extra code compared to more popular use cases supported by defaults.
  • NOT duplicate logic that belongs in Temporal's lower-level classes. This type should be a thin wrapper over the rest of Temporal, and it should delegate all heavy lifting to the other types. The only non-trivial logic in ZonedDateTime's implementation deals with RFC 5545 duration math or preventing inconsistencies in from and with.

Proposal Summary

ZonedDateTime is an ergonomic wrapper for use cases where the time zone is known. It models a real-world event: something that happens at a specific instant (an Instant time) in a specific place (therefore in a specific TimeZone) with a human-friendly representation (a DateTime in a specific Calendar).

This proposal consists of:

  1. A new Temporal.ZonedDateTime type

    • Its data model is (Instant, TimeZone, Calendar) although the proposal's implementation also derives and caches a DateTime for easy access to that type's methods and fields.
    • Regardless of its internal data model, from the API user's perspective a ZonedDateTime acts like the union of Instant, TimeZone, and DateTime.
    • Its API is a union of DateTime and Instant, with only a few changes:
      • inTimeZone is removed - switching time zones is done via with({timeZone})
      • adds toInstant() and toDateTime() downscaling methods
      • no new parameters on any method, although new options were added to some methods like add and subtract.
      • added a few convenience properties like hoursInDay and offsetString.
      • adds a timeZone field. This field will be accepted by with and from and emitted by getFields.
      • adds a offsetNanoseconds field for disambiguation and round-trip serialization via objects. This field will be accepted by with and from and emitted by getFields.
    • Most methods are trivial, 1-3 line delegations to other Temporal classes, but there are two hard parts that are detailed in the Discussion section later in this writeup:
      • Handling Conflicts and Ambiguity in from and with
      • add/subtract/difference using exact, date/time, or hybrid (RFC 5545) durations
    • For compatibility with the DateTime API, non-ISO calendars are supported. See the "Non-ISO Calendars" section below for details.
  2. Links between ZonedDateTime and the rest of Temporal

    • This proposal adds methods to now, DateTime, Instant, Date, Time, and TimeZone that make it easy to get a ZonedDateTime instance from another type.
    • A few places in Temporal may want to take advantage of the new type, either to remove redundant functionality (e.g. parsing/emitting IANA time zone suffixes in Instant) or to improve ergonomics (e.g. the relativeTo option in Duration's proposed balancing/rounding method.)
    • See the "Integration With the Rest of Temporal" section below for more details.

Discussion

Handling Conflicts and Ambiguity in from and with

ZonedDateTime internally can be defined with just an Instant, TimeZone, and Calendar, but externally it exposes all DateTime fields too. So from the user's perspective it's a mix of all four types, which presents a challenge for from and with because conflicts are possible.

Conflicts between time zone and offset are unavoidable, because time zone definitions can change between the time that a ZonedDateTime is serialized and when it's deserialized. See below for detailed discussion of options we'll provide for dealing with these conflicts.

Other conflicts will be prevented by only allowing fields that don't overlap so can't conflict: DateTime fields, offsetNanoseconds, and timeZone.

String Initializer behavior in from

  • ISO strings without a bracketed time zone suffix (IANA name or offset) will cause a RangeError, including UTC strings ending in "Z". Time zone is always required. For UTC behavior, use timeZone: 'UTC' or Instant.
    • There's been discussion about whether ISO "Z" strings should be parsed or thrown by ZonedDateTime. It's an interesting theoretical argument about whether "Z" in that context means "UTC time zone" or "zero offset". But IMHO, regardless of its theoretical meaning, allowing "Z" strings will introduce bugs where developers are surprised that all DateTime fields are "wrong" because (unlike legacy Date) the local time zone is not automatically inferred. To avoid this class of bugs, ZonedDateTime always requires the time zone to be set explicitly, including for UTC values.
    • Using offset "time zones" is an unusual case (and often a programmer error) so therefore therefore we require the offset to be repeated in brackets to ensure that the developer is opting in to this behavior. If you disagree with this behavior, comment in Extended ISO string representation if time zone is an offset (not an IANA name) #703. The brackets will also be emitted by toString() for round-trippable serialization.
  • Optional & required parts in ISO strings match DateTime's optional parts. Offset and calendar are also optional. Bracketed IANA time zone (or bracketed offset) is required.
  • It's possible for the time zone offset and the IANA time zone to conflict, e.g. ISO strings for future summertime events that were stored before a country permanently abolished DST. (@gibson042 I think mentioned this case re: Brazil abolishing DST.) In this case, the offset option (offset?: 'use' | 'prefer' | 'ignore' | 'reject' (default)) is used to resolve the conflict.
    • reject throws a RangeError, which matches behavior elsewhere in Temporal. This is the default to let developers know when their data is ambiguous or erroneous.
    • The default use option parses the Instant of the ZonedDateTime instance solely from the date/time fields and the offset. The time zone is still parsed and persisted, but when the instant is matched with the time zone, its local time could be different than what is shown in the ISO string. use is the default because it's guaranteed to be the same UTC time, which is critical for near-future events like scheduled tasks which might be missed if UTC timestamps changed. Also, this default ensures that an ISO string that was valid when persisted will always parse into a valid ZonedDateTime. Finally, use avoids DST ambiguity which is important because a likely reason for this case is a change in DST rules. EDIT 2020-09-02: default was changed to 'reject'.
    • ignore does the reverse: keeps local time constant and ignores the offset. This results in a different UTC time than when the ISO string originally stored. This option is useful for far-future events (e.g. conference agendas) where maintaining local time is important.
    • prefer will use the offset if it's a valid offset for the given time zone, but if it's not valid then the time zone (and the disambiguation option if present) will be used. This option is mainly helpful in with where it's the default. (See below.)

Object Initializer behavior in from

  • Accepted fields include:
    • all DateTime fields including calendar
    • timeZone - an IANA identifier or TimeZone instance
    • instant - an ISO string ending in "Z" or an Instant instance
    • offsetNanoseconds - this is used as an ergonomic alternative to disambiguation in cases where the offset is known, so that users won't have to manually determine which disambiguation to choose to get the desired offset in the result.
  • timeZone and either year/month/day fields or instant must be present. Other fields are optional.
  • It's OK if redundant fields are provided, e.g. instant and year. But redundant fields must match in the given time zone. Any conflicting values (e.g. different years in year vs. instant) will cause a RangeError to be thrown, except...
    • offsetNanoseconds and timeZone can conflict, just like with ISO strings. Conflicts are handled identically to how they're handled for ISO strings, using the offset option. See the ISO parsing section above for more details.
  • If neither instant nor offsetNanoseconds is not provided, then the time can be ambiguous around DST transitions. The disambiguation option (disambiguation?: 'compatible' (default) | 'earlier' | 'later' | 'reject') resolves this ambiguity.

from does not balance. This matches the behavior of DateTime and other non-Duration Temporal types. Math should be done with add and subtract.

with acts like from with only the following exceptions:

  • If timeZone and/or calendar fields are provided, then a new ZonedDateTime will be created (using the new timeZone/calendar fields if in input, or the old ones if not) and then other input fields will be applied on top of the new timeZone and/or calendar values. This matches the behavior of DateTime.prototype.with({calendar}). UPDATE 2020-10-11 We decided to limit timezone-changing to a withTimeZone method.
  • The default for offset is 'prefer'. This default enables users to make small changes to the time without unexpectedly changing the hour due to disambiguation (because the offsetNanoseconds value will be retained if it's still valid) but still allows larger changes (e.g. multiple hours or days) without an exception. For example, if a Temporal.ZonedDateTime is set to the "second" 1:30AM on a day where the 1-2AM clock hour is repeated after a backwards DST transition, then calling .with({minute: 45}) will result in an ambiguity which is resolved using the default offset: 'prefer' option. Because the existing offset is valid for the new time, it will be retained so the result will be the "second" 1:45AM. However, if the existing offset is not valid for the new result (e.g. .with({hour: 0})), then the offset will be changed.

If the offsetNanoseconds field is not provided, then the existing offsetNanoseconds field will be used by with as if it had been provided by the caller. By default, this will prefer the existing offset when resolving ambiguous results.

withTimeZone can be used to change the time zone while keeping the instant constant. The timeZone and calendar fields are not allowed as input to with. To keep clock time as-is while resetting the time zone, different code is required. Examples:

const sameInstantInOtherTz = zdt.withTimeZone('Europe/London');
const newTzSameLocalTime = zdt.toDateTime().toZonedDateTime('Europe/London');

Kinds of Durations & RFC 5545 Duration Math

Central to the design of (and justification for) ZonedDateTime is the complexity of using add, subtract, and difference in the face of three kinds of durations that can be stored in a Temporal.Duration:

  • An "exact duration" counts real-world elapsed time between two instants, like a stopwatch does. To get an exact duration, use the difference() method of Temporal.Instant.
  • A "date/time duration" counts the difference in calendar days and wall-clock time. To get a date/time duration, use the difference() method of Temporal.DateTime, Temporal.Date, Temporal.Time, or Temporal.YearMonth.
  • A "hybrid duration" (or "RFC 5545 duration") counts date differences in calendar days (like a date/time duration) but counts time differences in real-world elapsed time (like an exact duration). When persisting a duration without additional information about the kind it represents, then a hybrid duration is a good compromise choice because it usually matches end-users' expectations. For example, users typically want short durations (e.g. 15 minute calendar reminder) to ignore DST transitions, but also want full-day durations (e.g. 7 days later) to avoid changing the local time after addition or subtraction. Hybrid durations are specified by the RFC 5545 (iCalendar) standard.

All three kinds of durations usually have the same value. But if a DST transition (or other timezone-offset change) happened during the duration, then the date/time duration will be smaller or larger than the corresponding exact duration. This occasional difference can lead to bugs like showing reminders 60 minutes late on some mornings, or charging a customer for a 2-day rental on the 25-hour day when DST ends in the Fall.

Hybrid (RFC5545) durations in particular are very challenging without `ZonedDateTime, for two reasons:

  • DST edge cases - hybrid durations require round-trip conversions (Instant=>DateTime=>Instant) which require dealing with DST-related edge cases. ZonedDateTime tries to handles these cases in an RFC5545-compliant way.
  • Math Ergonomics - arithmetic operations with hybrid durations require knowing both the instant and the time zone, and usually require a round-trip conversion between Instant and DateTime. With existing Temporal types, this is a lot of extra code and opportunities for bugs.

EDIT 2020-09-02: We decided that ZonedDateTime supports only hybrid (RFC 5545) durations.

Therefore, ZonedDateTime supports one-line-of-code arithmetic on any of the three duration kinds, via a durationKind option on math methods for picking the type of duration. hybrid is the default because it satisfies the most real-world use cases and is safest for users who don't understand DST very well. But the other kinds are always available for opt-in use.

An alternate approach to a durationKind option would be to use Temporal types to determine duration behavior, e.g. ZonedDateTime math methods would use hybrid durations only, DateTime math methods would use date/time durations, and Instant math methods would use exact durations. The problem: a "use different types" approach requires the DateTime or Instant result to be converted back to ZonedDateTime. This conversion is sometimes lossy (if from DateTime) and is always ergonomically awful because the same time zone must be repeated. Also, novice or lazy developers may not convert back to ZonedDateTime, leaving later code vulnerable to DST bugs. To avoid data loss and conversion-related bugs, it's important to keep developers in the world of ZonedDateTime unless they explicitly opt out of it. For this reason, having a durationKind option in ZonedDateTime math methods is preferable to relying on different Temporal types to choose the duration kind for math operations.

Hybrid math issues and resolutions for DST edge cases:

  • ISSUE: RFC5545 requires a largest-to-smallest order of operations, which add (on Instant and DateTime types) currently uses but subtract does not. See Inconsistent order of operations for Date and DateTime plus and minus #653 for a discussion.
    • Resolution: This is not specific to ZonedDateTime-- the same question applies to DateTime, Date, etc. And there's a good reason for subtract to reverse the order of operations, because that enables more-reversible math operations, e.g. A + B - B => A. We've gotten tentative agreement with Neil Jenkins of the JSCalendar team that JSCalendar will match the reversed order-of-operations of subtraction in Temporal. See RFC 5545 vs DST questions for IETF calendar standards ("calext") group #702.
  • ISSUE: RFC5545 requires adjusting the date part of durations for DST (specifically by adding/subtracting any time zone offset delta between start and end), but not the time portion. How should we handle the case when there's a DST transition in the middle of the duration, either inside the date portion, inside the time portion, or at the border between the date and time portion?
  • ISSUE: In 2011, Samoa changed its time zone to skip an entire day by moving the country to the other side of the International Date Line.
    • RESOLUTION: add and difference will increment results by a full day if the result is on the other side of the missing day. subtract will decrement by one day for the same reason. This isn't special-case logic; instead it "just works" as long as transitions are understood to happen immediately before 0:00 and not at 0:00.
    • RESOLUTION: hoursInDay will return 24 for both the day before and the day after the missing day, because this property is always calculated as the number of real-world hours between midnight today and midnight tomorrow using 'later' disambiguation. If a day is skipped, the next midnight disambiguates to 2 days-from-now midnight, which is still 24 real-world hours from today midnight.
  • ISSUE: Time zone offset transitions can sometimes happen exactly at midnight, e.g. Atlantic/Azores
    • RESOLUTION: local midnight is never used in ZonedDateTime math operations.
    • RESOLUTION: hoursInDay will consider DST transitions at midnight as if they happened at the end of the previous calendar day, because the discontinuity actually happens at one instant before midnight, not between midnight and the next tick.
  • ISSUE: If two ZonedDateTime instances are in different time zones, then how to handle difference given that the same days in each time zone can be different lengths?
    • RESOLUTION: a RangeError will be thrown when largestUnit is 'days' or larger and the time zones have different name fields.
    • RESOLUTION: The docs will recommend alternative solutions for cross-timezone difference calculations:
      • If you need weeks, months, or years in the result, or if you need to take DST transitions into account, transform one of the instances to the other's time zone using .with({timeZone: other.timeZone}) and then calculate the same-timezone difference.
      • If you really only care about hours or smaller units, use .toInstant().difference(other.toInstant()), as long as it's OK if all days are assumed to be 24 hours long, DST is ignored, and you don't need units larger than hours.
      • To calculate with calendar dates only, use .toDate().difference(other.toDate()).
      • To calculate with clock times only, use .toTime().difference(other.toTime()).
  • ISSUE: What's the best default for largestUnit of difference ?
    • RESOLUTION: Because of the complexity and ambiguity involved in cross-timezone calculations, hours is the default for largestUnit.

Non-ISO Calendars

To support the DateTime compatibility goal of this type, and to keep things simple for non-ISO developers, ZonedDateTime matches DateTime's non-ISO calendar behavior:

  • ZonedDateTime.prototype.calendar field can be set in the constructor, from, or with
  • The calendar field is used to set the calendar of any DateTime instances created inside ZonedDateTime.
  • A withCalendar method clones the instance with the new calendar field set. It behaves the same as .with({calendar}).
  • from accepts a calendar and delegates to DateTime.prototype.from using that calendar.
  • with accepts a calendar and also delegates to DateTime, with any additional input fields are played on top of a new instance in the new calendar. The same approach is used for a new instant and/or a new timeZone.
  • getFields and getISOCalendarFields is delegated to DateTime but a few ZonedDateTime fields are appended before returning. Results will include calendar from DateTime.
  • All properties that come from DateTime delegate to their corresponding DateTime properties.
  • All calendar-sensitive math operations are delegated to DateTime.
  • ZonedDateTime makes no assumptions about length of any unit except that Instant days are 24 hours long.

ZonedDateTime behavior not discussed above

The sections above discuss the add, subtract, difference, with, and from methods. This section outlines the rest of ZonedDateTime, almost all of which is trivial wrapper methods which delegate to other Temporal types.

  • Constructor - takes an epochNanoseconds bigint, a TimeZone, and optional Calendar (default to iso8601). This proposal's code also derives and caches a DateTime from these three values to avoid having to create a new DateTime with most method calls.
  • getFields() - delegates to DateTime.prototype.getFields and appends timeZone, and offsetNanoseconds.
  • getISOCalendarFields() - same as above but delegates to DateTime.prototype.getISOCalendarFields.
  • Convenience property getters: offsetString, offsetNanoseconds, isoffsetTransition, hoursInDay
  • compare delegates to Instant.prototype.compare().
  • equals - normally delegates to Instant.prototype.equals() but will return false if timeZone.id and calendar.id don't match too. To check for UTC equality, use .toInstant().equals().
  • toLocaleString - the time zone is automatically set in options before delegating to Instant.prototype.toLocaleString(). If a time zone option is passed by the caller, then a RangeError will be thrown if the timezone doesn't match the current instance's time zone. Time zone conversion should be separate from time zone formatting.
  • toString/toJSON - string representation has the timezone offset, time zone identifier, and optionally calendar appended to the DateTime string representation.
  • toDateTime() - convenience method. This isn't a .dateTime property to subtly discourage users from calling DateTime methods instead of ZonedDateTime methods which will be safer for DST and not lossy.
  • All epochXxx fields from Instant are exposed via getters that delegate to Instant getters
  • All DateTime-like fields are exposed via getters that delegate to DateTime getters
  • The following methods also delegate to DateTime: toDate(), toYearMonth(), toMonthDay(), toTime().
  • valueOf() - throws

Conversions and Integration With the Rest of Temporal

To make it easy to interop between ZonedDateTime and other Temporal types, this PR proposes:

  • Temporal.now would get a zonedDateTime() method.
  • DateTime, Date, Time, and Instant would each get a toZonedDateTime() method to convert the current type to ZonedDateTime.
  • TimeZone would get a getZonedDateTimeFor() analogous to getInstantFor and getDateTimeFor.

All these methods have a similar shape:

  • The first parameter is an IANA string or timezone protocol. It's required in all types except for now where it's optional for compatibility with its siblings.
  • For methods on now and Instant types, the last parameter is an optional calendar name or protocol, like other methods that convert Instant->DateTime.
  • For methods on Date/DateTime/Time types, the last parameter is an options bag defaulting to disambiguation: 'compatible' to enable DST disambiguation like other methods that convert DateTime->Instant.
  • Date and Time methods have a Time or Date parameter, in between the time zone and options. For the Date version, the time is optional and will default to midnight. The Time version's Date parameter will be required.
  • All these methods return an instance of ZonedDateTime.

Signatures:

  • Temporal.now - zonedDateTime(tzLike: TimeZoneProtocol | string, calendar?: CalendarProtocol | string)
  • DateTime - toZonedDateTime(tzLike: TimeZoneProtocol | string, options?: ToInstantOptions)
  • Instant - toZonedDateTime.toZonedDateTime(tzLike: TimeZoneProtocol | string, calendar?: CalendarProtocol | string)
  • Date - toZonedDateTime(tzLike: TimeZoneProtocol | string, temporalTime?: Temporal.Time, options?: ToInstantOptions)
  • Time - toZonedDateTime(tzLike: TimeZoneProtocol | string, temporalDate: DateLike, options?: ToInstantOptions)
  • TimeZone - getZonedDateTimeFor(instant: Temporal.Instant, calendar?: CalendarProtocol | string)

Changes and Usage Elsewhere in Temporal

Once ZonedDateTime exists, we may choose to change or remove functionality elsewhere in Temporal that now makes more sense to live inside ZonedDateTime. Also, any Temporal features that need both a TimeZone and a Instant or a DateTime are candidates to use ZonedDateTime.

Prior Art

I'm new here so don't know much history. Feel free to fill in!

As I understand from reading old GitHub issues, there were once ZonedDateTime and ZonedInstant types but they were removed because of a number of problems. I tried to identify these problems from reading GitHub history and then tried to address them in this PR. But I'm sure I missed some! If you know from Temporal history about a problem with the last go-round that I missed here, please share!

@sffc recently proposed a ZonedInstant type. This PR shamelessly copies his idea, works through corner cases, and then adds 3-duration-kinds support and non-ISO calendars.

@pipobscure
Copy link
Collaborator

pipobscure commented Jun 25, 2020

This is significantly different than either ZonedAbsolute or ZonedDateTime. It seems to be more like a ZonedAbsoluteDateTime which attempts to contain and harmonise all the types Timezone, Absolute and DateTime.

I’ve only gone through this once, so this is a very raw impression: I actually like this. At a first glance mixing the three types rather than just two might give this proposal the missing je ne sais quois that made previous two-mix approaches fail.

TL;DR - 👍let’s think this through and discuss further

@InExtremaRes
Copy link

InExtremaRes commented Jun 26, 2020

@justingrant Hi. I'm curious, why do you pick LocalDateTime as a name? If ZonedDateTime haven't been "occupied", were you choose that instead? I know you said is just a placeholder name, but I'd like to know your rationale behind that as, you know, names communicates a lot the intentions of a model.

@justingrant
Copy link
Collaborator Author

justingrant commented Jun 26, 2020

@InExtremaRes - Great question. I avoided Zoned* names because of the historical baggage, because this type (like @pipobscure noted) wasn't a close match to those earlier proposals, and finally because a goal of this type is to be friendly and enticing for new users who may not know which Temporal type is the right one for their use case. "Zoned" is not particularly friendly, and it's not even a real English word so isn't obvious for new users. "Local" is a familiar word that has the connotation of "in a particular place" which is closer to what I was hoping to achieve with a name. But really I just wanted a non-confusing placeholder until we had time to pick a final name.

This probably isn't the last feedback about naming, so I opened up a new issue. If anyone has more naming feedback, let's discuss further it over in #707.

@codecov
Copy link

codecov bot commented Jun 28, 2020

Codecov Report

Merging #700 into main will decrease coverage by 52.34%.
The diff coverage is 3.26%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #700       +/-   ##
===========================================
- Coverage   94.23%   41.88%   -52.35%     
===========================================
  Files          18       18               
  Lines        6335     3261     -3074     
  Branches      957      715      -242     
===========================================
- Hits         5970     1366     -4604     
- Misses        358     1682     +1324     
- Partials        7      213      +206     
Flag Coverage Δ
#test262 41.88% <3.26%> (-4.00%) ⬇️
#tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
polyfill/lib/date.mjs 43.56% <0.00%> (-48.90%) ⬇️
polyfill/lib/datetime.mjs 50.83% <0.00%> (-44.50%) ⬇️
polyfill/lib/instant.mjs 45.97% <0.00%> (-51.43%) ⬇️
polyfill/lib/now.mjs 43.75% <0.00%> (-56.25%) ⬇️
polyfill/lib/time.mjs 44.50% <0.00%> (-52.83%) ⬇️
polyfill/lib/timezone.mjs 46.21% <0.00%> (-49.12%) ⬇️
polyfill/lib/zoneddatetime.mjs 3.75% <3.75%> (ø)
polyfill/lib/intl.mjs 12.14% <0.00%> (-87.86%) ⬇️
polyfill/lib/ecmascript.mjs 42.42% <0.00%> (-53.32%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c62e76d...d6f7f36. Read the comment docs.

Copy link
Collaborator

@ptomato ptomato left a comment

I have not really looked at the implementation at this stage, but concentrated on how the examples needed to be adapted and on the API that we expose. I do think this API nicely solves the problem outlined in #569 but my impression is that it needs some work to get it to the point where it is as streamlined as the other Temporal types. I gave some suggestions for bits that seemed to me like they could be removed in favour of something else.

docs/cookbook/getInstantBeforeOldRecord.mjs Outdated Show resolved Hide resolved
docs/cookbook/getLocalizedArrival.mjs Show resolved Hide resolved
docs/cookbook/localTimeForFutureEvents.mjs Outdated Show resolved Hide resolved
docs/cookbook/meetingPlanner.js Outdated Show resolved Hide resolved
docs/cookbook/meetingPlanner.js Outdated Show resolved Hide resolved
poc.d.ts Outdated Show resolved Hide resolved
poc.d.ts Outdated Show resolved Hide resolved
poc.d.ts Outdated Show resolved Hide resolved
poc.d.ts Outdated
getYearMonth(): Temporal.YearMonth;
getMonthDay(): Temporal.MonthDay;
getTime(): Temporal.Time;
valueOf(): never;
Copy link
Collaborator

@ptomato ptomato Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL never

This should probably be added to the other types in index.d.ts

Copy link
Collaborator Author

@justingrant justingrant Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, agreed. never is the right type for a method that always throws an exception.

@justingrant
Copy link
Collaborator Author

justingrant commented Jul 30, 2020

To prepare for our review meeting tomorrow, I just updated the proposal doc above with the latest details and open issues.

@justingrant
Copy link
Collaborator Author

justingrant commented Aug 10, 2020

Updates are now checked in for the two significant LocalDateTime decisions from our 2020-07-31 meeting:

  1. Removed durationKind option from math methods. Now all LocalDateTime math will use hybrid durations. Part of this was removing the options param from compare.
  2. Removed absolute property in favor of toAbsolute() (for reading) and Temporal.Absolute.toLocalDateTime() (instead of using absolute in from and with).

There's three more LocalDateTime updates planned for this week:
3) Changing the constructor to accept bigint instead of Absolute
4) Removing overridden order-of-operations code because we agreed with the IETF JSCalendar team that Temporal.DateTime's order of operations (addition: largest-units-first, subtraction: smallest-units-first) is the right way to do it and the JSCalendar spec will be updated to match Temporal's behavior.
5) Update the difference() polyfill implementation to match algorithm recommendations from JSCalendar folks.
6) Maybe: update the plus/minus algorithm depending on feedback from JSCalendar team.

justingrant added 13 commits Oct 21, 2020
* new Temporal.now.zonedDateTimeISO method
* require calendar in Temporal.now.zonedDateTime
* Update tests to handle default cal changes
* Add missing TS type for ZDT now methods
* Shorten `offsetString` property to just `offset`
* Remove `offsetNanoseconds` from ZDT fields. Replace it with `offset`.
* NOTE: subminute offsets aren't supported yet, but will be later (tc39#935)
This commit implements the largest-units-first behavior defined in tc39#993.
This includes both addition and subtraction.

Note that until tc39#993 is implemented, this commit inefficiently calls
`DateTime.prototype.add` (or `subtract`) multiple times. This can be
removed once tc39#993 lands.
Adds full rounding support to difference(). Fixes tc39#1023. Notes:
* See tc39#1023 for details on the algorithm.
* I added a bunch of new test cases for various DST-related edge cases.
* I suspect this algorothm can be simplified; it's not particularly DRY.
* This commit does *not* enable DST awareness in the `round` method.
  `round` currently works, but without any DST support which is OK for
  the time being. We can PR a DST-aware `round` in the next few weeks.
* The algorithm and/or implementation can probably be adapted for later
  use in `round`, as well as Duration's `add`, `subtract`, and `round`
  when `relativeTo` is a ZDT instance.  I can help with this next week.
* Docs updates for ZDT are not included in this commit. I'll file a new
  PR for docs changes next week.
justingrant added 2 commits Oct 21, 2020
Rounding is currently implemented by delegating down to DateTime, so I
only added a few sanity checks to make sure that basic rounding works.
In a later PR we'll add DST-aware rounding and that will be the time to
beef up tests.
@justingrant
Copy link
Collaborator Author

justingrant commented Oct 21, 2020

ZonedDateTime is ready for handoff for a real implementation! Whoo-hoo! The last big missing piece was rounding support in difference() and that's now done. All tests pass except 5 calendar localization tests (unrelated to ZDT) that behave differently on my local machine than in CI. I also gave it a fresh rebase.

You can look at the recent commit titles to see which of the recent Temporal-wide decisions have already been implemented in the code. I've been trying to keep up but things have been moving pretty fast so a few things are behind main. Here's the things I know about that are NOT done yet:

I put some minor docs updates (e.g. renaming offsetString=>offset) into a separate PR: #1034.

@ptomato - Let me know how I can best help you! I'm happy to meet in real time this afternoon or tomorrow if there are things you want to go over. Also, if you'd like me to implement any of the items above on top of the current implementation, just let me know. Otherwise I'll plan to avoid new commits on this branch to stay out of your way.

You can ignore the /polyfill/poc folder that contains TS source and build-steps used to transform TS to .mjs and .d.ts. I'm assuming as soon as you take ownership of the code that all subsequent work will be done directly in .mjs and .d.ts and the TS source will be retired.

@ryzokuken ryzokuken linked an issue Oct 22, 2020 that may be closed by this pull request
@ptomato
Copy link
Collaborator

ptomato commented Oct 22, 2020

This is fine, thanks. I will start on this as soon as I've closed out 3 other issues today. I assume that the discussions have put us enough on the same page about this that a call won't be necessary, but I'll let you know if I do have questions.

ptomato added a commit that referenced this issue Nov 2, 2020
These conversions are redundant, they either go through ZonedDateTime or
TimeZone.

Cookbook examples that used these conversions are changed, where possible,
to use ZonedDateTime. Where that is not possible because of unimplemented
ZonedDateTime methods, we add a FIXME note and fall back to the more
verbose TimeZone conversion methods.

Updating some of the cookbook examples is taken from #700.

Co-authored-by: Justin Grant <justin@justingrant.net>

Closes: #1026
ptomato added a commit that referenced this issue Nov 2, 2020
These conversions are redundant, they either go through ZonedDateTime or
TimeZone.

Cookbook examples that used these conversions are changed, where possible,
to use ZonedDateTime. Where that is not possible because of unimplemented
ZonedDateTime methods, we add a FIXME note and fall back to the more
verbose TimeZone conversion methods.

Updating some of the cookbook examples is taken from #700.

Co-authored-by: Justin Grant <justingrant@users.noreply.github.com>

Closes: #1026
Ms2ger pushed a commit that referenced this issue Nov 2, 2020
These conversions are redundant, they either go through ZonedDateTime or
TimeZone.

Cookbook examples that used these conversions are changed, where possible,
to use ZonedDateTime. Where that is not possible because of unimplemented
ZonedDateTime methods, we add a FIXME note and fall back to the more
verbose TimeZone conversion methods.

Updating some of the cookbook examples is taken from #700.

Co-authored-by: Justin Grant <justingrant@users.noreply.github.com>

Closes: #1026
@ptomato
Copy link
Collaborator

ptomato commented Nov 6, 2020

With #1153 I believe we have got everything needed from this PR, so I wll close this now.

@ptomato ptomato closed this Nov 6, 2020
@justingrant
Copy link
Collaborator Author

justingrant commented Nov 6, 2020

Wow, it's been a long road with this one! I've been so grateful to be able to help out here. It was fun!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants