Fix ZDT diff issue with ymd travel with opposite sign of ns travel#3317
Fix ZDT diff issue with ymd travel with opposite sign of ns travel#3317arshaw wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3317 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 22 22
Lines 10763 10775 +12
Branches 1866 1868 +2
=======================================
+ Hits 10590 10602 +12
Misses 161 161
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ptomato
left a comment
There was a problem hiding this comment.
And another big thank you for looking into this one as well!
I haven't yet had a chance to dig as deep into this issue as I have into the other ones, so I'll have less to say.
Firefox's proposed patch also had something similar to the first part of yours, falling back to time math if isoDtStart/isoDtEnd passed each other. (They had a second fix in ZonedDateTime.round which I didn't believe was correct though.)
This PR passes the tentative test262 tests I've written, but note that I still need to validate those against the ideas that Justin wrote down in #3311 (comment).
I threw a bunch of snapshot tests at this PR and noticed that it does change one result that I wasn't expecting:
const earlier = Temporal.ZonedDateTime.from('2011-12-29T23:59:59.999999999-10:00[Pacific/Apia]');
const later = Temporal.ZonedDateTime.from('2011-12-31T00:00+14:00[Pacific/Apia]');
earlier.since(later, { largestUnit: 'days' })
// was : P1DT0.000000001S
// becomes: PT0.000000001SThis is indeed falling back to time math, but here I'm not sure that it should, since it's a +24h transition where the direction of travel of wall clock and exact time is the same.
| const isoDtLexSign = CompareISODate(isoDtStart.isoDate, isoDtEnd.isoDate); | ||
|
|
||
| // If year-month-day values are same-day, | ||
| // there's no point involving the calendar/timezone for zdt->pdt conversions. | ||
| // It also avoids a situation where dayCorrection backs up too far on same-day diffs | ||
| // with reverse-direction wallclock delta due to DST: | ||
| // https://github.com/tc39/proposal-temporal/issues/3141 | ||
| if ( | ||
| isoDtStart.isoDate.year === isoDtEnd.isoDate.year && | ||
| isoDtStart.isoDate.month === isoDtEnd.isoDate.month && | ||
| isoDtStart.isoDate.day === isoDtEnd.isoDate.day | ||
| // If year-month-day values are same-day, | ||
| // there's no point involving the calendar/timezone for zdt->pdt conversions. | ||
| // It also avoids a situation where dayCorrection backs up too far on same-day diffs | ||
| // with reverse-direction wallclock delta due to DST: | ||
| // https://github.com/tc39/proposal-temporal/issues/3141 | ||
| isoDtLexSign === 0 || | ||
| // If year-month-day diff is opposite sign from overall nanosecond diff, then the time zone | ||
| // likely repeated a whole day (ex: America/Adak). In this case, do all math in time realm. | ||
| // https://github.com/tc39/proposal-temporal/issues/3311 | ||
| isoDtLexSign !== sign | ||
| ) { |
There was a problem hiding this comment.
These two conditions can probably collapse into if (CompareISODate(isoDtStart.isoDate, isoDtEnd.isoDate) === -sign)?
Fix for #3311
Just some strategically placed short-circuits guarding against weird scenarios, falling back to time-only math.
Output:
All results look reasonable, except for maybe the first .total one. Gotta think through if that's correct. Might be related to another PR I'm about to make...