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

Add "kinds of durations" section to Duration docs #639

Closed
wants to merge 2 commits into from

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Jun 2, 2020

Per the discussion in #559, Duration is challenging to make DST-safe because (unlike DateTime vs. Absolute), there's no way to know that the duration in a particular Duration instance represents an "absolute" duration or a "date/time" duration.

This PR adds a new section in the summary of the Duration documentation that:

  • explains the difference between "absolute" and "date/time" durations (UPDATE: I'm planning to add more content about the third kind: RFC 5545 durations that combine a "date/time" date part and an "absolute" time part)
  • explains why that difference is important for correctly dealing with DST issues
  • provides 3 simple best practices to follow:
    • Don't assume that every day is 24 hours long.
    • Only use absolute durations with Temporal.Absolute methods.
    • Don't combine different kinds of durations.
  • provides code samples illustrating correct and incorrect application of each best practice.

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #639 into main will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #639      +/-   ##
==========================================
+ Coverage   97.82%   97.84%   +0.02%     
==========================================
  Files          16       16              
  Lines        3727     3765      +38     
  Branches      637      638       +1     
==========================================
+ Hits         3646     3684      +38     
  Misses         79       79              
  Partials        2        2              
Flag Coverage Δ
#test262 54.56% <71.42%> (+0.05%) ⬆️
#tests 92.87% <100.00%> (+0.07%) ⬆️
Impacted Files Coverage Δ
polyfill/lib/absolute.mjs 100.00% <100.00%> (ø)
polyfill/lib/date.mjs 93.88% <100.00%> (+0.08%) ⬆️
polyfill/lib/datetime.mjs 95.65% <100.00%> (+0.03%) ⬆️
polyfill/lib/duration.mjs 98.29% <100.00%> (+0.01%) ⬆️
polyfill/lib/ecmascript.mjs 97.78% <100.00%> (+<0.01%) ⬆️
polyfill/lib/monthday.mjs 99.13% <100.00%> (+0.06%) ⬆️
polyfill/lib/slots.mjs 100.00% <100.00%> (ø)
polyfill/lib/time.mjs 99.33% <100.00%> (+<0.01%) ⬆️
polyfill/lib/yearmonth.mjs 97.43% <100.00%> (+0.12%) ⬆️

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 4c260c2...8048452. Read the comment docs.

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Thanks!

Thinking about Temporal's strong typing approach, the words "There are two kinds of durations" seem to indicate to me that we really should have two duration types, and coincidentally that ties in with today's comment on Duration.compare: #608 (comment)

I'm OK with merging this anyway, though, and thinking about splitting Duration into two types in another issue (much as I dislike the idea of adding more types...) Any thoughts from others?

docs/duration.md Show resolved Hide resolved
docs/duration.md Outdated Show resolved Hide resolved
@justingrant
Copy link
Collaborator Author

justingrant commented Jun 2, 2020

Thinking about Temporal's strong typing approach, the words "There are two kinds of durations" seem to indicate to me that we really should have two duration types, and coincidentally that ties in with today's comment on Duration.compare: #608 (comment)

Yep, I agree that this inconsistency is surprising given how strict the rest of Temporal is about avoiding ambiguity.

...thinking about splitting Duration into two types in another issue (much as I dislike the idea of adding more types...)

Yeah, this PR was both a way to help initial users and as a not-so-subtle hint that documentation may not be enough to solve this problem. ;-)

At a high level, there are three possible solutions:

  1. Keep Duration type as-is, but provide docs and/or opt-in API methods (e.g. TimeZone.prototype.getAbsoluteDurationFor(dtDuration)) for easier cross-kind conversion.
  2. Differentiate the kinds in API (e.g. separate classes and/or a kind field) and persistence (e.g. a "Z" suffix for absolute durations)
  3. Specify that a Duration has a specific meaning, e.g. "always absolute" or "always datetime" (or, like RFC 5545 apparently does, a mix where time units are absolute while date units are not!). Then change API to prevent storing the "wrong" meaning.

I've drafted a write-up of 10-ish total variations on these three basic approaches, each with its own pros and cons. It's kinda long. What's the best venue to discuss? The discussion currently seems spread across a few GH issues, none of which are exactly focused on the core "Durations can mean two things" problem.

Co-authored-by: Philip Chimento <[email protected]>
@justingrant
Copy link
Collaborator Author

justingrant commented Jun 3, 2020

Ugh, I was reminded today that there's really a third kind of Duration: an RFC-5545 (iCalendar) duration, where the date portion is a "date/time" duration but the time portion is an "absolute" duration.

Even outside of the iCalendar standard, AFAIK this is a common pattern. For example, moment.js uses it, at least when adding/subtracting single units because it tends to match most users' expectations: that short time intervals (e.g. 15-minute event reminder time) are calculated in absolute time, while longer durations (e.g. 2-day event) are calculated in a way that keeps local times unchanged when adding/subtracting durations.

Anyway, tomorrow I'll take a second look at this docs content in this PR and see if there's an obvious way to communicate how to work with this third kind of duration without overcomplicating things too much.

@ptomato
Copy link
Collaborator

ptomato commented Jun 3, 2020

Is that really a third kind of Duration? It seems to me even in a "nominal" duration, the hours/minutes/seconds are still "accurate"?

@justingrant
Copy link
Collaborator Author

@ptomato Is that really a third kind of Duration? It seems to me even in a "nominal" duration, the hours/minutes/seconds are still "accurate"?

Yep, it's different, although only in a narrow case: when you want the time portion of a duration to measure clock time not absolute time. Here's an example:

// "fall back" DST transition in America/Los_Angeles
fallBack = Temporal.Absolute.from('2020-11-01T09:00Z') 
fallBack.minus({hours:1}).toString()
// => "2020-11-01T08:00Z"
fallBack.inTimeZone('America/Los_Angeles').minus({hours:1}).inTimeZone('America/Los_Angeles').toString()
// => "2020-11-01T07:00Z"

If we decided that the case of "clock time durations" wasn't important to support (FWIW, clock time durations aren't supported in Java nor .NET, and seem relatively uncommon in real-world use cases-- so this may not be as crazy as it sounds) then we could simply specify that all Temporal durations' time portion was exact/absolute, and the only difference in meaning between the kinds is the meaning of the date portion.

Was that what you were suggesting?

If yes, that'd imply that we'd have to do one or both of the following:

  • DateTime.prototype.difference would have to accept a TimeZone parameter to allow the clock time portion of the result to be converted into absolute time
  • split DateTime.prototype.difference into timeDifference() and dateDifference(), with the former method accepting a TimeZone parameter.

Also, if a time-only (or date/time) duration meant only absolute time, then DateTime.prototype.plus and DateTime.prototype.minus would need to take a TimeZone parameter.

@justingrant
Copy link
Collaborator Author

I decided to delay updates to these docs until I finished the JSDoc and implementation of RFC5545 duration handling in the ZonedAbsolute prototype's difference/plus/subtract methods. Once I'm confident that everything looks good there, I'll simplify the learnings into the docs and samples in this PR. Should be ready within the next few days.

@ptomato
Copy link
Collaborator

ptomato commented Aug 13, 2020

The information in this PR seems to be outdated in light of LocalDateTime — should we close it or try to revise it?

@ptomato
Copy link
Collaborator

ptomato commented Jan 15, 2021

Closing as I think this is no longer relevant as mentioned in #639 (comment)

@ptomato ptomato closed this Jan 15, 2021
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.

3 participants