Initial implementation of ZonedDateTime #1073

ptomato · 2020-10-29T05:14:44Z

This includes all the "trivial" methods of ZonedDateTime and their tests (spec text already exists), and all the conversion methods from other types to ZonedDateTime, and their tests and specification text.

Unimplemented methods (with, add, subtract, until, since, round, getFields, and anything dealing with DST) throw, for the time being. Some options (offset and overflow in from()) are also not implemented yet.

Time.toZonedDateTime was missing the options argument. Calendar.dateUntil had the wrong variable names.

…tion Since the allowed values for roundingIncrement are identical in DateTime.round() and ZonedDateTime.round(), the tedious list of ifs can go into one operation.

Make the calendar argument in DateTime align with the other types, and use the new conversion symbols for Instant.epochSeconds and friends.

codecov · 2020-10-29T05:16:16Z

Codecov Report

Merging #1073 into main will decrease coverage by 1.28%.
The diff coverage is 84.30%.

@@            Coverage Diff             @@
##             main    #1073      +/-   ##
==========================================
- Coverage   94.88%   93.59%   -1.29%     
==========================================
  Files          18       19       +1     
  Lines        7134     7702     +568     
  Branches     1137     1224      +87     
==========================================
+ Hits         6769     7209     +440     
- Misses        358      486     +128     
  Partials        7        7

Flag	Coverage Δ
#test262	`41.11% <7.50%> (-4.51%)`	⬇️
#tests	`89.54% <78.81%> (-1.64%)`	⬇️

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

Impacted Files	Coverage Δ
polyfill/lib/zoneddatetime.mjs	`69.13% <69.13%> (ø)`
polyfill/lib/ecmascript.mjs	`96.54% <98.75%> (+0.51%)`	⬆️
polyfill/lib/date.mjs	`93.61% <100.00%> (+0.55%)`	⬆️
polyfill/lib/datetime.mjs	`95.69% <100.00%> (+0.04%)`	⬆️
polyfill/lib/duration.mjs	`97.54% <100.00%> (+0.08%)`	⬆️
polyfill/lib/instant.mjs	`96.91% <100.00%> (+0.13%)`	⬆️
polyfill/lib/slots.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/temporal.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/time.mjs	`97.61% <100.00%> (+0.11%)`	⬆️

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 643fd26...1cb5160. Read the comment docs.

ptomato · 2020-10-29T05:19:47Z

The only discrepancy so far in the implemented parts with the TypeScript implementation that I'm aware of is 2020-03-08T09:00Z[America/Los_Angeles]. I can't remember if we discussed this somewhere, but we have so far treated "Z" as a time zone designator, not an offset, see #313.

polyfill/index.d.ts

justingrant

This is amazing! I started reviewing tonight, will finish tomorrow. Looks good so far.

polyfill/index.d.ts

justingrant · 2020-10-29T09:07:43Z

The only discrepancy so far in the implemented parts with the TypeScript implementation that I'm aware of is 2020-03-08T09:00Z[America/Los_Angeles]. I can't remember if we discussed this somewhere, but we have so far treated "Z" as a time zone designator, not an offset, see #313.

Yep, I put that in for better compatibility with Java's parser which treats Z as an offset.

Regardless of what we do with ZDT, I don't think we should be accepting Z strings in TimeZone.from. #1075

Here's context around the Java thing:

Note that current Temporal parsing regexes fail when Z is used instead of a numeric offset (e.g. 2020-03-08T09:00Z[America/Los_Angeles]). This is why we can't parse the whole original string into an Instant. But the Java.time parser accepts "Z" as an offset, so for compatibility and ergonomics we do too. Below is a quote from Jon Skeet (creator of Joda Time which java.time was based on) that may explain why Java accepts this format. From https://stackoverflow.com/a/61245186:

It's really unfortunate that ISO-8601 talks about this as a time zone,
when it's only a UTC offset - it definitely doesn't specify the actual
time zone. (So you can't tell what the local time will be one minute
later, for example.)

justingrant · 2020-10-29T09:09:14Z

This is so cool. Amazing progress. I started reviewing tonight, will plan to finish tomorrow.

polyfill/lib/ecmascript.mjs

spec/zoneddatetime.html

polyfill/lib/zoneddatetime.mjs

ptomato · 2020-10-29T17:41:52Z

Thanks for the reviews, both!

The only discrepancy so far in the implemented parts with the TypeScript implementation that I'm aware of is 2020-03-08T09:00Z[America/Los_Angeles]. I can't remember if we discussed this somewhere, but we have so far treated "Z" as a time zone designator, not an offset, see #313.

Yep, I put that in for better compatibility with Java's parser which treats Z as an offset.

OK, I'll address this in the next PR (which will also enable +06:00[+06:00] anyway), but will leave this as is in this PR, and just fix the other comments.

This contains all the "easy" ZonedDateTime methods, and their tests, as well as all the methods to convert from other types to ZonedDateTime and their tests and specification. See: #569

justingrant

This looks great. I tried to give this (esp. the tests and the polyfill implementation) a close review. Found a few issues but overall LGTM. Thanks!

justingrant · 2020-10-30T06:35:46Z

polyfill/lib/date.mjs

@@ -16,6 +16,7 @@ import {
  NANOSECOND,
  DATE_BRAND,
  CALENDAR,
+  EPOCHNANOSECONDS,


Minor question: why is it TIME_ZONE but EPOCHNANOSECONDS? Is there some meaning intended by not splitting EPOCH and NANOSECONDS with an underscore?

I think it's pretty arbitrary. Splitting with an underscore seems proper, but I didn't want to rename all of the existing EPOCHNANOSECONDS names used in Instant.

justingrant · 2020-10-30T06:47:59Z

polyfill/lib/ecmascript.mjs

+    if (ES.Type(item) === 'Object') {
+      if (ES.IsTemporalZonedDateTime(item)) return item;
+      calendar = item.calendar;
+      if (calendar === undefined) calendar = new (GetIntrinsic('%Temporal.ISO8601Calendar%'))();


Just to confirm: Temporal.ISO8601Calendar is internal-facing only, right? Regular users won't see it when they type Temporal. in browser dev tools? If so, that's great. I assume that this is currently the plan per the Sept 18 meeting discussion of #292, but then seeing the code above I wasn't 100% sure.

To be decided in #847

justingrant · 2020-10-30T06:50:48Z

polyfill/lib/ecmascript.mjs

+        offset,
+        calendar
+      } = ES.ParseTemporalZonedDateTimeString(ES.ToString(item)));
+      if (!ianaName) throw new RangeError('named time zone required');


This error message is a little misleading in the case where there's an offset in brackets. How about 'time zone ID required in [brackets]' or something like that.

justingrant · 2020-10-30T07:11:13Z

polyfill/lib/zoneddatetime.mjs

+    ES.ValidateTemporalUnitRange(largestUnit, smallestUnit);
+    let roundingMode = ES.ToTemporalRoundingMode(options, 'nearest');
+    roundingMode = ES.NegateTemporalRoundingMode(roundingMode);
+    const maximumIncrements = {


move this to module scope to avoid repeating it 2x?

justingrant · 2020-10-30T07:13:42Z

polyfill/lib/zoneddatetime.mjs

+    const tz = GetSlot(this, TIME_ZONE);
+    return {
+      calendar: GetSlot(this, CALENDAR),
+      hour: GetSlot(dt, HOUR),


will ZDT or time calendars land first? If the latter, then should these be isoHour, isoMinute, etc?

Apparently ZDT :)

Cool, @ptomato wins the game of PR chicken! ;-)

justingrant · 2020-10-30T07:28:52Z

polyfill/test/zoneddatetime.mjs

+    it('casts its argument', () => {
+      equal(`${zdt.withCalendar('japanese')}`, '2019-11-18T15:23:30.123456789-08:00[America/Los_Angeles][c=japanese]');
+    });
+    it('keeps instant and calendar the same', () => {


You mean time zone, not calendar in the title of this test.

justingrant · 2020-10-30T07:38:38Z

polyfill/test/zoneddatetime.mjs

+        })
+      );
+    });
+    it('at least the required properties must be present', () => {


Should there be another test that verifies that the minimal required properties can successfully be parsed? For example, I'd assume that the calendar property is optional in this context. Would be good to have a test to verify that.

Might also make sense to verify that the minimum set really is the minimum set. For example, try deleting each property from the bag one at a time and verify that it throws every time.

justingrant · 2020-10-30T07:47:23Z

spec/instant.html

@@ -181,8 +181,8 @@ <h1>get Temporal.Instant.prototype.epochSeconds</h1>
        1. Let _instant_ be the *this* value.
        1. Perform ? RequireInternalSlot(_instant_, [[InitializedTemporalInstant]]).
        1. Let _ns_ be _instant_.[[Nanoseconds]].
-        1. Let _s_ be RoundTowardsZero(_ns_ / 1,000,000,000<sub>ℝ</sub>).
-        1. Return the Number value for _s_.
+        1. Let _s_ be RoundTowardsZero(ℝ(_ns_) / 1,000,000,000<sub>ℝ</sub>).


I noticed these symbols "𝔽" and ℝ in the spec when I was working on the PR for lessThan/greaterThan/etc. What do they mean? Also, what does the "?" (or, more rarely, "!") mean before a function call?

See https://tc39.es/ecma262/#%E2%84%9D and https://tc39.es/ecma262/#sec-returnifabrupt-shorthands

justingrant · 2020-10-30T07:55:45Z

polyfill/index.d.ts

+    readonly daysInYear: number;
+    readonly monthsInYear: number;
+    readonly inLeapYear: boolean;
+    readonly startOfDay: Temporal.ZonedDateTime;


Ahh, crap. This may run afoul of the guidelines we've been using that object-valued property getters must return the same (===) object every time. That'd imply a slot to cache this object. Probably not worth the hassle and potential runtime performance impact to keep it a property. Should we change this to a startOfDay() method?

Good catch, this is better off as a method.

justingrant · 2020-10-30T08:01:55Z

polyfill/lib/zoneddatetime.mjs

+  }
+  toLocaleString(locales = undefined, options = undefined) {
+    if (!ES.IsTemporalZonedDateTime(this)) throw new TypeError('invalid receiver');
+    return new DateTimeFormat(locales, options).format(this);


All the toLocaleString logic happens over in Intl? I'm particularly wondering about the case where the time zone of the receiver conflicts with the time zone in the options. #700 forces the instance's time zone into the options bag and throws an exception if the user put a timezone in the options that's different from the instance's time zone. I think (not 100% sure) matches how calendar conflicts are handled which seems like a reasonable behavior to match.

Yeah, the check is in PartitionDateTimePattern; we wouldn't want toLocaleString to be inconsistent with manually using DateTimeFormat.

justingrant · 2020-10-30T08:10:13Z

Haha @Ms2ger I keep losing races with you to finish reviewing PRs before they're merged! Next time I'll ping you if I'm in the middle of a long late-night (for me) review. No worries in this PR though, it looks solid and the issues I found can all be addressed in later PRs.

ptomato · 2020-10-30T17:17:29Z

I will be addressing all these remarks in my next pull request.

ptomato added 5 commits October 28, 2020 11:03

Fix optional argument to Date.toDateTime() in TypeScript

97d5cb2

Documentation fixes

fe6c384

Time.toZonedDateTime was missing the options argument. Calendar.dateUntil had the wrong variable names.

Refactor DateTime and ZonedDateTime rounding increment into one opera…

ff514e8

…tion Since the allowed values for roundingIncrement are identical in DateTime.round() and ZonedDateTime.round(), the tedious list of ifs can go into one operation.

Spec fixes

c9d11a4

Make the calendar argument in DateTime align with the other types, and use the new conversion symbols for Instant.epochSeconds and friends.

Use ToTemporalCalendar in DateTime.withCalendar

f03b29f

ptomato requested review from Ms2ger, justingrant, pipobscure, jasonwilliams, sffc, gibson042, mattjohnsonpint and ryzokuken October 29, 2020 05:14

justingrant reviewed Oct 29, 2020

View reviewed changes

polyfill/index.d.ts Outdated Show resolved Hide resolved

justingrant reviewed Oct 29, 2020

View reviewed changes

polyfill/index.d.ts Outdated Show resolved Hide resolved

justingrant reviewed Oct 29, 2020

View reviewed changes

polyfill/index.d.ts Outdated Show resolved Hide resolved

Ms2ger requested changes Oct 29, 2020

View reviewed changes

Ms2ger added the awaiting-author label Oct 29, 2020

ptomato force-pushed the zoneddatetime-1 branch from 01af565 to bd12c6e Compare October 29, 2020 17:42

ptomato removed the awaiting-author label Oct 29, 2020

ptomato requested a review from Ms2ger October 29, 2020 17:43

Initial ZonedDateTime implementation

1cb5160

This contains all the "easy" ZonedDateTime methods, and their tests, as well as all the methods to convert from other types to ZonedDateTime and their tests and specification. See: #569

ptomato force-pushed the zoneddatetime-1 branch from bd12c6e to 1cb5160 Compare October 29, 2020 17:59

Ms2ger approved these changes Oct 30, 2020

View reviewed changes

Ms2ger merged commit 56dee7e into main Oct 30, 2020

Ms2ger deleted the zoneddatetime-1 branch October 30, 2020 07:16

justingrant approved these changes Oct 30, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of ZonedDateTime #1073

Initial implementation of ZonedDateTime #1073

ptomato commented Oct 29, 2020

codecov bot commented Oct 29, 2020 •

edited

Loading

ptomato commented Oct 29, 2020

justingrant left a comment

justingrant commented Oct 29, 2020

justingrant commented Oct 29, 2020

ptomato commented Oct 29, 2020

justingrant left a comment

justingrant Oct 30, 2020

ptomato Oct 30, 2020

justingrant Oct 30, 2020

Ms2ger Oct 30, 2020

justingrant Oct 30, 2020

justingrant Oct 30, 2020

justingrant Oct 30, 2020

Ms2ger Oct 30, 2020

justingrant Oct 30, 2020

justingrant Oct 30, 2020

justingrant Oct 30, 2020

justingrant Oct 30, 2020

Ms2ger Oct 30, 2020

justingrant Oct 30, 2020

ptomato Oct 30, 2020

justingrant Oct 30, 2020

Ms2ger Oct 30, 2020

justingrant commented Oct 30, 2020

ptomato commented Oct 30, 2020

Initial implementation of ZonedDateTime #1073

Initial implementation of ZonedDateTime #1073

Conversation

ptomato commented Oct 29, 2020

codecov bot commented Oct 29, 2020 • edited Loading

Codecov Report

ptomato commented Oct 29, 2020

justingrant left a comment

Choose a reason for hiding this comment

justingrant commented Oct 29, 2020

justingrant commented Oct 29, 2020

ptomato commented Oct 29, 2020

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

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

justingrant commented Oct 30, 2020

ptomato commented Oct 30, 2020

codecov bot commented Oct 29, 2020 •

edited

Loading