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

Options for handling "3 kinds of durations" problem #686

Closed
justingrant opened this issue Jun 18, 2020 · 9 comments
Closed

Options for handling "3 kinds of durations" problem #686

justingrant opened this issue Jun 18, 2020 · 9 comments

Comments

@justingrant
Copy link
Collaborator

The discussion in #559 and a few other issues highlighted a fundamental challenge with Duration: it's impossible to know what a Duration means without knowing how it was constructed. P1DT2H could have three different meanings:

  • 26 hours of clock time, aka "date/time duration"
  • 26 real-world hours, aka "absolute duration"
  • one calendar day plus 2 real-world hours, aka "hybrid duration" or "RFC 5545 duration".

This ambiguity is different from all other Temporal classes. If you have an Absolute, you know you're looking at a specific moment in time. If you have a Date, Time, DateTime, etc. you know you're looking at calendar/clock time. The persisted representations are also clear: you can tell from looking at an ISO 8601 date/time string that it's actual or nominal. But a Duration is always ambiguous. This makes it harder for developers to write DST-safe code.

Although docs (e.g. #639) can help, the problem remains that Duration lacks the separation of concerns and unambiguousness of DateTime and Absolute. So I think it's worth considering what else Temporal can do here.

The write-up below outlines possible options to solve this ambiguity.

At a high level, there are three options:

  1. Support all three kinds but leave it up to the developer to know which kind a particular Duration instance is. This is the current API. We could optionally provide options parameters and/or new methods to make it easier to convert the "wrong" types to the "right" type, like we do with Absolute<->DateTime conversions today.
  2. Provide a differentiated API for each kind of Duration, e.g. a kind field and/or separate classes. The kind field should be emitted by getFields for object persistence, and optionally we could extend the string persistence format too (e.g. a "Z" suffix for absolute durations).
  3. Define the meaning of Duration as one of the three kinds, and ensure that Duration-using methods enforce that meaning. For example, if Duration means "datetime duration" then Absolute.prototype.difference must require a TimeZone parameter.

Here's variations on each of these options to discuss::

Variations on: Option 1 - "Don't Change Duration data model nor API"

1a. Don't Change Any Temporal APIs on Any Type. Rely on documentation to explain the problem. IMHO this is inconsistent with DateTime vs. Absolute separation of concerns, but it's still a valid choice.

Pros:

  • No changes to Temporal API

Cons:

  • Many/most developers probably won't follow DST best practices

1b. Add Duration Conversion Methods - Provide methods on the TimeZone class that convert between the two types of durations, analogous to inTimeZone() for DateTime and Absolute.

Pros:

  • Minimal changes to Temporal API
  • Makes it a little easier to follow DST best practices

Cons:

  • Opt-in, so may not be used much
  • Hard to discover
  • Can't tell from persisted durations what kind it is.

1c. Conversion Methods + durationKind Option in Math Methods

  • Add duration conversion methods on TimeZone (same as 1b)
  • Also add a new durationKind option to plus, minus, and difference to tell Temporal about what kind of duration is in input (for plus/minus) or should be in output (for difference).
  • Also add a timeZone parameter to plus, minus, and difference methods. It'd be required if durationKind is the "wrong" kind, and ignored otherwise.

Pros:

  • Minimal changes to existing Temporal data model & API

Cons:

  • If the option is always required, this adds a large ergonomic tax to use-cases where the "right" kind is used. If optional, then it's not really much safer than current API, because users can still perform operations on the "wrong" kind.
  • Can't tell from persisted durations what kind it is.

1d. ZonedAbsolute + Defaults - Add a new class type that represents a particular instant at a particular place on earth, and leverage its time zone knowledge for better duration-kind-conversion ergonomics.

  • Add duration conversion methods on TimeZone (same as 1b)
  • Add options parameter (e.g. {durationKind: 'absolute' | 'dateTime' | 'hybrid'} to this class's plus/minus/difference methods to specify the kind of the input or output duration.
  • Consider making one of the kinds a default option (probably 'hybrid'), if we think it will work for a large majority of use cases. Developers could still opt into other kinds by changing the option.

Pros:

  • Much better ergonomics than (1b) because TimeZone parameters won't be required in math methods.
  • "It just works" defaults will help less-experienced developers follow DST best practices.

Cons:

  • Current plan of record is that there won't be a ZonedAbsolute.
  • Defaults won't be appropriate for some (admittedly uncommon) use cases, e.g. measuring a short time interval using clock time (not absolute time).
  • Developers who use Absolute or DateTime are out of luck.
  • Can't tell from persisted durations what kind it is.

Variations on: Option 2 - "Change the API and/or Data Model to Differentiate Duration Kinds"

2a. New XxxDuration classes - Clone the Duration type and use class names to clarify if a duration is "absolute", "date/time", or "hybrid".

  • Absolute.prototype.difference would return AbsoluteDuration. This class would only have hours or smaller units.
  • The difference methods of DateTime, Date, Time, and YearMonth would return a DateTimeDuration.
  • Add a kind: 'absolute' | 'datetime' | 'hybrid' field (name could be type, flavor, etc.) for use in object persistence with getFields() and from.
    • from and with would throw if the wrong kind field is provided for that class.
  • AbsoluteDuration would have time fields but no date fields, to more clearly communicate to users that "absolute days" don't really exist.
  • Add duration conversion methods on TimeZone (same as 1b)
  • Accept a TimeZone parameter on plus/minus methods, which would be required for durations of the wrong types but optional for durations of the right type. This would be a convenience to avoid developers having to call the kind-conversion methods on TimeZone before or after many calls to plus/minus/difference.
    • Note that a ZonedAbsolute type would not need this parameter because it already knows the time zone.
  • Property bags passed to plus or minus methods (or Duration.prototype.from or Duration.prototype.with) would be assumed to have the "right" type, unless they contain a kind of the wrong type. This supports common cases like .plus({days: 1}) without having to specify a kind, under the assumption that if developers build property bags themselves, then they know what they're doing.
  • XxxDuration classes' plus/minus/difference methods would throw if provided an instance of another kind.

Pros:

  • Consistent with the existing DateTime/Absolute split in Temporal, so will be relatively easy to explain and learn

Cons:

  • timeZone parameters are sometimes required and sometimes optional-- is that weird?
  • Can't tell from persisted durations what kind it is.

2b. New XxxDuration classes + ISO String Extensions - Same as 2a, but also extend persistence formats with duration kind info.

  • Absolute durations could get a "Z" suffix (like Absolute string persistence)
  • Date/time durations could use a "bare" 8601 syntax (like DateTime string persistence), or it could get a different suffix character like "C" (for "calendar/clock"), e.g. "P2DT12HC".
  • Hybrid is harder. One option would be to apply "Z" separately to the time and date portions, so P2DZT12HZ would represent a 36-hour absolute duration while P2DT12HZ would mean "2 calendar days and 12 absolute hours". A cleaner approach might be to use another suffix character, e.g. P2DT12HQ.
  • from methods would throw if provided a string of the wrong kind for that duration class.

Pros:

  • Same as 1a, and...
  • String-persisted durations would retain kind.

Cons:

  • Same as 1a, and...
  • Requires non-standard extensions to string persistence.

2c. Single Duration Class Type with kind Field
Like Option 2a, but rely on the kind field instead of class name to differentiate duration kinds.

  • Duration's constructor and from (using an object initializer) must require a kind parameter.
  • Otherwise same as Option 2a

Pros:

  • Can use Duration.from() without knowing in advance what kind of duration is input.
  • Won't add another type

Cons:

  • Same as 2a, and...
  • May be a somewhat harder to learn/explain compared to using class names that parallel the Absolute vs. DateTime split we already have.

2d. Single Duration Class Type with kind Field + ISO Persistence
Same as Option 2b, but rely on the kind field instead of the class type to differentiate different flavors of durations.

Pros:

  • Same as 2b, and...
  • Can use Duration.from() without knowing in advance what kind of duration is input
  • Won't add another type

Cons:

  • Same as Option 2b, and...
  • Somewhat harder to learn/explain compared to using class names that parallel the Absolute vs. DateTime split we already have.

Variations on: Option 3 - "Pick and enforce one of the three meanings for Duration"

3a. Specify that Duration Means "Absolute Duration" - Absolute's plus, minus, and difference methods would continue to accept & return Duration, while other types' plus, minus, and difference methods would add a required timeZone parameter. (Except ZonedAbsolute wouldn't require a timeZone parameter because it knows its time zone.)

Pros:

  • Unambiguous in code or when persisted to objects or strings
  • No change needed to string persistence format
  • Great ergonomics for ZonedAbsolute

Cons:

  • Impossible to persist a DST-safe duration, e.g. "1 week". This is probably a deal-breaker.
  • Bad ergonomics for non-ZonedAbsolute types : adds required timeZone parameter to almost every Temporal method that accepts or returns a duration.

3b. Specify that Duration is Only "DateTime Duration" - This is the inverse of Option 3a. Absolute's plus/minus/difference methods would get a required timeZone parameter, while all other types would stay as-is.

Pros:

  • Unambiguous in code or when persisted to objects or strings
  • No change needed to string persistence format
  • Few/no changes to existing APIs outside Absolute
  • Good ergonomics on the DateTime/Date/Time/etc side of Temporal
  • Great ergonomics for ZonedAbsolute

Cons:

  • Lossy because of DST. A 1-hour real-world difference starting when DST ends would convert to a zero-length datetime duration. This is probably a deal-breaker.

3c. Specify that Temporal Duration matches RFC 5545: date fields represent a DateTime difference, while time fields represent an Absolute difference - This combines Option 3a and 3b and matches how RFC 5545 defines a duration: a combination of a date/time date-only duration and an absolute time-only duration. So PT2H would be considered absolute time, P2D would be two calendar days, and P2DT2H would mean 2 calendar days plus 2 real-world hours.

Pros

  • Unambiguous in code or when persisted to objects or strings
  • Aligns with RFC 5545 and downstream standards like CalDAV or JSCalendar. This helps developers building or interoperating with calendar apps.
  • Matches user expectations in most use cases, which is that short periods (e.g. 15-minute meeting reminder) are real-world stopwatch times, while longer periods (e.g. 3 days) try to keep local times unchanged when added or subtracted. I assume that this is why RFC 5545 chose this convention.
  • Matches moment.js behavior for math operations
  • Great ergonomics for ZonedAbsolute
  • Developers who want absolute-duration semantics can simply store the duration with hours as the largest unit.

Cons

  • A TimeZone and a starting date would be required for Duration's plus/minus/difference
  • A TimeZone would be required for all non-Duration types' plus/minus/difference methods (except ZonedAbsolute)
  • Creates discontinuities when time is added to a date-only duration, or vice versa.
  • Clock time durations cannot be persisted. That said, intra-day clock time duration persistence is a relatively unusual use-case relative to others.
@sffc
Copy link
Collaborator

sffc commented Jul 10, 2020

The current Temporal.Duration was designed as a "bag of options" with the knowledge that a duration is very much context-dependent. The actual amount of time represented by a Temporal.Duration depends on:

  1. The start date (does "+1 month" mean 28, 29, 30, or 31 days?)
  2. The calendar system (does "+1 month" mean a Hebrew or a Gregorian month?)
  3. Wall clock or absolute (does "+1 day" mean "+24 hours" or "+1 day counter"?)

Your proposals are various attempts at fixing issue 3. Should we also fix issues 1 and 2 at the same time?

The intent with the current design is that the programmer declares all of the context via the type upon which the Temporal.Duration is applied. If you apply it to an Absolute, you get absolute math, and if you have years or months in your Temporal.Duration, you get an exception. If you apply it to a DateTime, you get wall clock math, and you inherit the start date and calendar system from the receiver.

Furthermore, I think it's safe to say that the primary purpose of having Absolute and DateTime is so that you can pick how you want to do arithmetic.


I think that this problem needs to be seen in context with ZonedAbsolute/LocalDateTime.

My current opinion on this matter is:

  1. If we adopt LocalDateTime as an additional class, I prefer 1d. The DST safety problem is solved by adding another core type. I could also live with @ryzokuken's version of option 2 (BigInt for absolute durations and Temporal.Duration for wall clock durations).
  2. If we adopt LocalDateTime and throw away Absolute and DateTime, I prefer 3c. I think 3c is most self-consistent with that data model.
  3. If we do not adopt LocalDateTime, I prefer 3c, with a RangeError if you pass the wrong fields to the wrong type. We should also investigate whether Temporal.Duration should carry a time zone and/or calendar system in its data model in order to resolve the fields correctly. I could also live with @ryzokuken's version of option 2 (BigInt for absolute durations and Temporal.Duration for wall clock durations).

@sffc
Copy link
Collaborator

sffc commented Jul 10, 2020

Some observations:

  • A calendar is required for all arithmetic with large date fields (months and higher).
  • A time zone is required for DST-safe wall clock arithmetic with time fields (hours and smaller).
  • Wall clock arithmetic with days does not require either a time zone or a calendar.
  • Absolute arithmetic with hours and smaller does not require either a time zone or a calendar.

@ryzokuken
Copy link
Member

If we do not adopt LocalDateTime, I prefer 3c, with a RangeError if you pass the wrong fields to the wrong type. We should also investigate whether Temporal.Duration should carry a time zone and/or calendar system in its data model in order to resolve the fields correctly. I could also live with @ryzokuken's version of option 2 (BigInt for absolute durations and Temporal.Duration for wall clock durations).

These are also my preferences.

@justingrant
Copy link
Collaborator Author

@ryzokuken - do you also want to weigh in on #759 which is a more detailed proposal for Option 3c?

BigInt for absolute durations and Temporal.Duration for wall clock durations

I assume this would mean that Absolute.prototype.difference would return a bigint?

What I wasn't sure about: would Absolute.prototype.plus / Absolute.prototype.minus only accept accept bigint arguments? Or would it accept Duration instances, Duration-like objects, and bigint values?

I'd recommend the latter because (unlike bigints used to initialize Absolute instances), durations are frequently created as literal values, e.g. 1 hour. Large bigint literals are problematic:

  • bigint literals don't support exponential notation, so you can't add an hour via abs.plus(3.6e12n). You'd have to do abs.plus(BigInt(3.6e12n)).
  • It's hard to read code that uses large numbers, e.g. 144000000000000n. I know that the _ separator can help, but still it's so much less clear than {hours: 4}.

This is similar to (but 1e6 worse than) the problem that exists today with legacy Date and milliseconds, where every Date-using app and library (including Temporal!) has its own collection of constants like MSECS_PER_HOUR. I'm hoping that this practice won't be needed with Temporal.

@justingrant
Copy link
Collaborator Author

justingrant commented Jul 12, 2020

@sffc - rolling up several posts' worth of responses here:

The intent with the current design is that the programmer declares all of the context via the type upon which the Temporal.Duration is applied.

My main concern with the current design is that it's too easy to accidentally move Durations between contexts. For example, a function is written that expects a DateTime or Absolute duration, but the caller passes the "wrong" type. This is an easy mistake to make, as we saw from @ptomato's cookbook bug. It's not a big deal if the context is in the next line of code, but when passing Durations into far-away context (e.g. libraries, code you didn't write, etc.) it will be really easy to mix up contexts, esp. because the bugs will only be obvious during 2 hours per year.

Wall clock arithmetic with days does not require either a time zone or a calendar.

I've been thinking lately that the best way to handle these use cases is via math methods on the Date type. If you see code like foo.toDate().difference(other) then it's really clear that only dates will come back, and it's always safe for DST. So although date-only math is safe for a DateTime, the easy alternative using Date makes me relatively unworried about removing DateTime math or making it harder to use by requiring a time zone parameter.

If we do not adopt LocalDateTime, I prefer 3c, with a RangeError if you pass the wrong fields to the wrong type.

One benefit of Option 3c, even if we have LocalDateTime, is that the durationKind option goes away, which will make LocalDateTime significantly simpler, both to implement and to explain to users. Another benefit is.... ↓↓↓

  1. The start date (does "+1 month" mean 28, 29, 30, or 31 days?)
  2. The calendar system (does "+1 month" mean a Hebrew or a Gregorian month?)
  3. Wall clock or absolute (does "+1 day" mean "+24 hours" or "+1 day counter"?)

Your proposals are various attempts at fixing issue 3. Should we also fix issues 1 and 2 at the same time?

I believe that Option 3c fixes all three of these, per bullet 5.5 in #759. Let me know if I missed something about fixing (1) and (2).

@justingrant
Copy link
Collaborator Author

One more note: even if Absolute still retains the ability to use the Duration type, having a way to turn a Duration into a bigint could be useful. For example, it'd provide a way to compare two durations to make up for (see #608) the lack of Duration.prototype.compare.

@rowaasr13
Copy link

This discussion seems to be most relevant to what I wanted to do with Duration and is unable to: I want to use it as typing tool to provide unambiguous duration argument to function that is supposed to issue commands to different backends that themselves use different types - e.g. one takes milliseconds and another nanoseconds. So I could write something like do_some_stuff_for_specific_duration({backend: 1, command: "frobnicate", duration: Temporal.Duration.from({ seconds: 5 }) without having to remember what time units my own API uses. With that I could write inside that function logic that pulls correct units from Duration for each backend.

It seems that current Duration implementation is not suitable for this. It is missing ability to pull any totals mentioned in previous comment and in #584. Additionally it is ambiguous for all units from day and above. I've seen Time workaround with largestUnits in #730, but Time is time, not a duration - mixing two is wrong at least semantically (and probably have problems with time beyond 23:59).

It looks to me there should be some other Temporal.<something> type for that, so current Duration API won't become unnecessarily convoluted. It should be limited to hours and below. Or maybe days should be allowed too with strict documentation that for this type day is always equal to 24 hours.

@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

What's left to discuss on this issue? It seems to me that the round() method and relativeTo solve at least some of the concerns here.

@ptomato
Copy link
Collaborator

ptomato commented Jan 14, 2021

I believe all the issues with Durations described above have been addressed by the changes in October/November. The idea is that the duration means something different depending on which type you use it with.

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

No branches or pull requests

5 participants