Implement ZonedDateTime.until() and since() in polyfill #1116

ptomato · 2020-11-04T18:37:40Z

Note that the DifferenceZonedDateTime abstract operation is not final:
there is one edge case that is broken in the polyfill, which is that the
since() method needs to use this as the relative date for rounding.
There is a test for that which is skipped.

~~A follow-up commit will fix this case and add spec text for the algorithm.~~

Co-authored-by: Justin Grant [email protected]

See: #569

codecov · 2020-11-04T18:44:15Z

Codecov Report

Merging #1116 into main will decrease coverage by 2.71%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1116      +/-   ##
==========================================
- Coverage   93.55%   90.83%   -2.72%     
==========================================
  Files          19       17       -2     
  Lines        7961     8741     +780     
  Branches     1264     1232      -32     
==========================================
+ Hits         7448     7940     +492     
- Misses        506      790     +284     
- Partials        7       11       +4

Flag	Coverage Δ
test262	`?`
tests	`90.83% <ø> (+1.39%)`	⬆️

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

Impacted Files	Coverage Δ
polyfill/lib/shim.mjs
polyfill/lib/legacydate.mjs
polyfill/lib/slots.mjs
polyfill/lib/intrinsicclass.mjs
lib/calendar.mjs	`77.59% <0.00%> (ø)`
lib/plaindatetime.mjs	`90.84% <0.00%> (ø)`
lib/temporal.mjs	`100.00% <0.00%> (ø)`
lib/ecmascript.mjs	`94.29% <0.00%> (ø)`
lib/plaindate.mjs	`87.08% <0.00%> (ø)`
lib/plaintime.mjs	`89.73% <0.00%> (ø)`
... and 11 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 4b94392...747fbbe. Read the comment docs.

justingrant

I don't have time today to closely review the algorithm line-by-line, but I left a bunch of comments, mostly related to our discussion in #993. If your plan is to address those comments in a follow-up, then this PR seems OK to merge now.

polyfill/lib/ecmascript.mjs

polyfill/lib/zoneddatetime.mjs

polyfill/lib/ecmascript.mjs

justingrant · 2020-11-04T20:02:05Z

polyfill/test/zoneddatetime.mjs

+    const later = ZonedDateTime.from('2019-10-29T10:46:38.271986102-03:00[America/Santiago]');
+    ['years', 'months', 'weeks', 'days', 'hours', 'minutes', 'seconds'].forEach((largestUnit) => {
+      const diff = later.since(earlier, { largestUnit });
+      it(`earlier.since(later, ${largestUnit}) == later.since(earlier, ${largestUnit}).negated()`, () =>


NOTE: This test won't work for largestUnit of 'years' or 'months' for some pairs of dates. If it works for this pair, that's just luck. The only guarantee that you can make given our decision of (A) in #993 (comment) is that if d = a.since(b) then a.subtract(d).equals(b). AFAIK there are no other guarantees that can be made for all pairs of values.

What I'm doing for the DateTime and Date versions of these tests is to run different tests for months/years vs. all other units. And I use pairs of values that trigger the behavior difference. If you're not seeing failures in this test, then I'd suggest changing your earlier/later values to ones where behavior will vary by direction.

I have changed this part to the following, which I think should be correct regardless of whether #993 has landed:

const earlier = ZonedDateTime.from('1976-11-18T15:23:30.123456789-03:00[America/Santiago]'); const later = ZonedDateTime.from('2019-10-29T10:46:38.271986102-03:00[America/Santiago]'); // The interchangeability of since() and until() holds for time units only ['hours', 'minutes', 'seconds'].forEach((largestUnit) => { const diff = later.since(earlier, { largestUnit }); it(`earlier.since(later, ${largestUnit}) == later.since(earlier, ${largestUnit}).negated()`, () => equal(`${earlier.since(later, { largestUnit })}`, `${diff.negated()}`)); it(`earlier.until(later, ${largestUnit}) == later.since(earlier, ${largestUnit})`, () => equal(`${earlier.until(later, { largestUnit })}`, `${diff}`)); it(`${largestUnit} difference symmetrical with regard to negative durations`, () => { assert(earlier.subtract(diff.negated()).equals(later)); assert(later.add(diff.negated()).equals(earlier)); }); }); // For all units, add() undoes until() and subtract() undoes since() ['years', 'months', 'weeks', 'days', 'hours', 'minutes', 'seconds'].forEach((largestUnit) => { const diff1 = earlier.until(later, { largestUnit }); const diff2 = later.since(earlier, { largestUnit }); it(`earlier.add(${diff1}) == later`, () => assert(earlier.add(diff1).equals(later))); it(`later.subtract(${diff2}) == earlier`, () => assert(later.subtract(diff2).equals(earlier))); });

If I have time later, I'll try to add a few test cases that trigger calendar or time zone differences.

Looks good. I did a similar split in #1123.

polyfill/test/zoneddatetime.mjs

justingrant · 2020-11-04T20:24:31Z

polyfill/test/zoneddatetime.mjs

+  describe('Maths', () => {
+    const earlier = ZonedDateTime.from('1976-11-18T15:23:30.123456789-03:00[America/Santiago]');
+    const later = ZonedDateTime.from('2019-10-29T10:46:38.271986102-03:00[America/Santiago]');
+    ['years', 'months', 'weeks', 'days', 'hours', 'minutes', 'seconds'].forEach((largestUnit) => {


I'd think you need 2-4 more variations of this test to test reversibility with DST involved. If there's disambiguation at the point where you're done adding days and need to add the time, then behavior will vary based on the direction of the operation. It's possible that those tests already exist, so can ignore this comment if so. But there's a lot of confusing logic around the interplay between dates, times, and DST and it's easy to get it wrong. I think that you'll know you have good values of earlier and later when a.add(d).equals(b)===true but b.subtract(d).equals(a)===false and the results are 1 or 2 hours apart.

For variations, it probably needs one for the transition on the earlier side, and one on the later side, and then a test of both forward and backward transitions.

I don't think I'll get this done tonight, but let's open an issue tomorrow to add more test coverage.

justingrant · 2020-11-04T20:43:36Z

polyfill/test/zoneddatetime.mjs

@@ -1473,6 +2124,263 @@ describe('ZonedDateTime', () => {
    });
  });

+  describe('math around DST', () => {


Given that since and until will have different behavior around DST transitions, some (all?) of the added tests below probably be cloned or refactored to test since too.

Ditto, I don't think I'll get this done tonight either, but let's include this in the issue to add more test coverage.

ptomato · 2020-11-04T22:50:25Z

I wasn't intending to take the changes from #993 into account in these tests when I wrote them yesterday, but maybe you're right that that would be better, or at least write them such that they would pass in both cases. I'll see about updating this after I'm finished with with().

justingrant · 2020-11-04T23:52:53Z

I wasn't intending to take the changes from #993 into account in these tests when I wrote them yesterday, but maybe you're right that that would be better

Sounds good. If you're not going to merge this PR today (which would probably be helpful for my PR anyways), then I'd suggest to merge the #993 fix in tonight/tomorrow and then you may be able to copy/paste both tests and logic from my PR if it helps accelerate things on the ZDT end. But I won't plan to make any ZDT changes in my PR. (Although I will be changing differenceDate and the subtract methods in various types so there still may be some tests that break on your end.) Does this sound like an OK plan?

ptomato · 2020-11-05T00:14:44Z

We do need to make absolutely certain this is merged tomorrow, so that I can cut a release of the polyfill in preparation for Friday. If it gets into conflict with your changes, then maybe I can remove the failing tests from this PR at that point so that we at least have an implementation that works with most options.

justingrant · 2020-11-05T05:02:13Z

Sounds good. I'm wrapping up #993 PR now... just need to delete a bunch of dateSubtract stuff out of Calendar. Should have it posted within the hour. BTW, turned out that #1111 didn't break any tests (at least not any that I know about!).

Also I did end up fixing a few ZDT things like enabling skipped order-of-operations tests and removing the subtraction AO. But otherwise steered clear of ZDT to reduce conflicts.

ptomato · 2020-11-05T05:51:03Z

I've managed to fix the edge case that was broken, so I've enabled the remaining test now.

justingrant · 2020-11-05T07:27:53Z

Heads up @ryzokuken and @Ms2ger - once #1123 is merged, it's likely that some tests in this PR will break because #1123 changes the behavior of subtract and since for most Temporal types. It shouldn't actually break this PR's implementation (unless this PR has a bug that was hidden by existing behavior) but some tests in this PR may need new "expected" data to match the (now correct) output of subtract, since, and ES.DateDifference from #1123.

I'll leave it up to you guys and @ptomato to figure out how to order the merges, but I assume it'd be better to merge #1123 before this PR because this #1123 is lower-level and larger in scope than this PR's changes. And also because it will probably be faster for @ptomato to fix any of this PR's tests that are broken by #1123 than for me to figure out why his tests are breaking! ;-)

justingrant

IMHO, the since implementation should be updated to match the #993 fixes. I commented inline where exactly the needed changes are, based on #1023's implementation of DateTime.prototype.since.

Otherwise this looks good!

polyfill/lib/zoneddatetime.mjs

polyfill/lib/ecmascript.mjs

ryzokuken

RSLGTM

Co-authored-by: Justin Grant <[email protected]> See: #569

ptomato · 2020-11-05T23:09:05Z

I believe this is updated to reflect the order-of-operations change, merging.

ptomato requested review from Ms2ger, justingrant and ryzokuken November 4, 2020 18:37

ptomato force-pushed the zoneddatetime-5 branch from 1de5bff to 7bb88d4 Compare November 4, 2020 18:43

justingrant approved these changes Nov 4, 2020

View reviewed changes

ptomato force-pushed the zoneddatetime-5 branch from 7bb88d4 to 066af40 Compare November 5, 2020 04:56

ptomato force-pushed the zoneddatetime-5 branch from 066af40 to d2e2457 Compare November 5, 2020 05:49

justingrant mentioned this pull request Nov 5, 2020

Fix order of operations in subtract and direction in since #1123

Merged

justingrant requested changes Nov 5, 2020

View reviewed changes

ryzokuken approved these changes Nov 5, 2020

View reviewed changes

Implement ZonedDateTime.until() and since() in polyfill

747fbbe

Co-authored-by: Justin Grant <[email protected]> See: #569

ptomato force-pushed the zoneddatetime-5 branch from d2e2457 to 747fbbe Compare November 5, 2020 22:45

ptomato merged commit 3aa42c3 into main Nov 5, 2020

ptomato deleted the zoneddatetime-5 branch November 5, 2020 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ZonedDateTime.until() and since() in polyfill #1116

Implement ZonedDateTime.until() and since() in polyfill #1116

ptomato commented Nov 4, 2020 •

edited

Loading

codecov bot commented Nov 4, 2020 •

edited

Loading

justingrant left a comment

justingrant Nov 4, 2020

justingrant Nov 4, 2020

ptomato Nov 5, 2020

justingrant Nov 5, 2020

justingrant Nov 4, 2020

ptomato Nov 5, 2020

ptomato Nov 5, 2020

justingrant Nov 4, 2020

ptomato Nov 5, 2020

ptomato commented Nov 4, 2020

justingrant commented Nov 4, 2020

ptomato commented Nov 5, 2020

justingrant commented Nov 5, 2020

ptomato commented Nov 5, 2020

justingrant commented Nov 5, 2020

justingrant left a comment •

edited

Loading

ryzokuken left a comment

ptomato commented Nov 5, 2020

Implement ZonedDateTime.until() and since() in polyfill #1116

Implement ZonedDateTime.until() and since() in polyfill #1116

Conversation

ptomato commented Nov 4, 2020 • edited Loading

codecov bot commented Nov 4, 2020 • edited Loading

Codecov Report

justingrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptomato commented Nov 4, 2020

justingrant commented Nov 4, 2020

ptomato commented Nov 5, 2020

justingrant commented Nov 5, 2020

ptomato commented Nov 5, 2020

justingrant commented Nov 5, 2020

justingrant left a comment • edited Loading

Choose a reason for hiding this comment

ryzokuken left a comment

Choose a reason for hiding this comment

ptomato commented Nov 5, 2020

ptomato commented Nov 4, 2020 •

edited

Loading

codecov bot commented Nov 4, 2020 •

edited

Loading

justingrant left a comment •

edited

Loading