ISO 8601 calendar #638

ptomato · 2020-06-01T19:55:48Z

This adds a built-in ISO 8601 calendar and delegates all calendar-sensitive operations to it.

This should be the last PR that needs to be merged before we can release the polyfill. It's ready for review and can be merged as-is, but here is a list of things I still intend to do, either in this pull request or a follow up:

Check if calendar.dateTimeFromFields can be removed
Add the in-progress Japanese calendar, if it's possible to get it into shape in time
Add tests for creating a custom calendar inheriting from Temporal.Calendar
Add tests for creating a custom calendar not inheriting from Temporal.Calendar

sffc · 2020-06-02T01:02:37Z

docs/calendar.md

+However, the ISO 8601 calendar is not the only calendar in common use in the world.
+Some places use another calendar system as the main calendar, or have a separate calendar system as a commonly-used civil or religious calendar.
+
+> **NOTE:** The Temporal polyfill currently only includes the ISO 8601 calendar, but the Temporal proposal will eventually provide all the calendars supported by Intl.


Can you ship the Japanese calendar in the polyfill since I think you already have it mostly done?

I thought we had mentioned not doing that and instead encouraging others to implement Intl calendars in order to exercise the API?

I can add it, but I think it might need a bit more work than I will be able to complete this week if we still want to keep to our deadline of "within the next few days". On the other hand, it would certainly be nice to have an actual non-ISO calendar that we could use in the docs examples, so that we can make the docs examples actually executable. I'll see how far I get.

docs/calendar.md

sffc · 2020-06-02T01:28:58Z

Looks quite good! I left some comments. I started looking through the code but my laptop was about to die so I thought it was better to send what I had.

ptomato · 2020-06-03T00:29:48Z

Thanks for the review, very helpful. There are still some to-do items on this branch that I won't get to today, but it's getting closer.

codecov · 2020-06-05T00:23:54Z

Codecov Report

Merging #638 into main will decrease coverage by 1.55%.
The diff coverage is 90.77%.

@@            Coverage Diff             @@
##             main     #638      +/-   ##
==========================================
- Coverage   95.99%   94.43%   -1.56%     
==========================================
  Files          17       17              
  Lines        4040     4760     +720     
  Branches      663      749      +86     
==========================================
+ Hits         3878     4495     +617     
- Misses        159      262     +103     
  Partials        3        3

Flag	Coverage Δ
#test262	`52.82% <53.83%> (-1.02%)`	⬇️
#tests	`89.35% <86.21%> (-1.85%)`	⬇️

Impacted Files	Coverage Δ
polyfill/lib/calendar.mjs	`65.01% <78.74%> (+37.74%)`	⬆️
polyfill/lib/datetime.mjs	`93.55% <91.31%> (-2.12%)`	⬇️
polyfill/lib/date.mjs	`93.43% <92.39%> (-0.46%)`	⬇️
polyfill/lib/intl.mjs	`98.73% <92.85%> (-1.27%)`	⬇️
polyfill/lib/ecmascript.mjs	`95.62% <94.85%> (-1.97%)`	⬇️
polyfill/lib/yearmonth.mjs	`97.81% <97.80%> (+0.38%)`	⬆️
polyfill/lib/monthday.mjs	`97.85% <98.03%> (-1.29%)`	⬇️
polyfill/lib/absolute.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/now.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/regex.mjs	`100.00% <100.00%> (ø)`
... and 5 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 81a4b67...915ae53. Read the comment docs.

sffc

Looking at more of the code.

polyfill/index.d.ts

sffc · 2020-06-09T21:52:25Z

polyfill/lib/calendar.mjs

+
+export class Iso8601 extends Calendar {
+  constructor(id = 'iso8601') {
+    // Needs to be subclassable, that's why the ID is a default argument


Not sure what to think here. On the one hand, new Iso8601() shouldn't need an ID argument. On the other hand, it seems like it should be required for subclasses to specify their ID.

I don't see a good alternative here if we want to store the ID in an internal slot of the calendar. That can only be done in super(), not in userland. To solve this we could drop the internal slot (just require an .id property on calendars), or disallow subclassing, or have ISO8601Base, IslamicBase, etc. calendars that you extend (but then we'd have to expose them to userland calendars as part of the API).

polyfill/lib/calendar.mjs

sffc · 2020-06-09T21:59:55Z

polyfill/lib/calendar.mjs

+  }
+  year(date) {
+    if (!ES.IsTemporalCalendar(this)) throw new TypeError('invalid receiver');
+    return GetSlot(date, ISO_YEAR);


I wonder if we should make an observable access to all three ISO slots, since other non-ISO calendar systems may need to access all three slots.

They can use date.getISOCalendarFields(), no?

Maybe we should read date.getISOCalendarFields() here as well, then, to reduce differences between calendars?

I might have misunderstood your original remark, but I'm not sure I understand what purpose that would serve. Compared with the old, pre-calendar situation, reading the internal slot directly doesn't introduce any observable accesses other than that of invoking the calendar getter itself. Calling the method means the spec will mandate that an observable call to getISOCalendarFields() is interposed every time you evaluate date.year.

The call to getISOCalendarFields() is unavoidable in userland calendars because they can't read slots, but all of the built-in calendars can at least just read the slots.

Right, and I was saying that maybe the built in calendars should also have an observable call to make it consistent with custom or polyfilled calendars. But I don't feel strongly; I was just making an observation.

sffc

Got through a few more files.

polyfill/lib/date.mjs

polyfill/lib/datetime.mjs

polyfill/lib/ecmascript.mjs

polyfill/lib/monthday.mjs

sffc

Phew, done with the library code. I'll mostly trust that the tests are correct.

polyfill/lib/regex.mjs

polyfill/lib/yearmonth.mjs

jasonwilliams · 2020-06-10T20:06:58Z

polyfill/lib/calendar.mjs

+
+export class Iso8601 extends Calendar {
+  constructor(id = 'iso8601') {
+    // Needs to be subclassable, that's why the ID is a default argument


Is there a reason we can't have an .id property on the class?

(you'd want it to be an accessor on the prototype that returned the ID string from an internal slot) this seems pretty important for Calendar objects to have.

See my reply to Shane's question above (#638 (comment)), if we want the ID in an internal slot then we do need to chain up in order to give userland subclasses a way to set that internal slot.

Sorry that was a response to this

I don't see a good alternative here if we want to store the ID in an internal slot of the calendar. That can only be done in super(), not in userland. To solve this we could drop the internal slot (just require an .id property on calendars), or disallow subclassing, or have ISO8601Base, IslamicBase, etc. calendars that you extend (but then we'd have to expose them to userland calendars as part of the API).

Whats the pros and cons of just doing this:

To solve this we could drop the internal slot (just require an .id property on calendars)

Would an .id property not be useful for userland code anyway?

(you'd want it to be an accessor on the prototype that returned the ID string from an internal slot) this seems pretty important for Calendar objects to have.

Hey @ljharb could you elaborate on this a bit more?
If the user is setting the id on their own subclass why does this need to be the case? (Why does it need to be an internal_slot over a property)
Thanks

They can override whatever they want to on their subclass - but the ID seems like a pretty intrinsic and immutable characteristic of a calendar to me, and those things generally aren’t data properties.

Certainly, like all of our builtin classes, subclasses should/must use super() to get all the appropriate internal slots, and I’d expect the ID to be provided to the base constructor (to store it in the internal slot)

IIUC no change is necessary then? We have an .id property, but the ID is stored in an internal slot of the calendar, which is achieved by calling super(id) in the constructor of your calendar. The only ugliness is this default argument on the ISO8601 calendar's constructor which I think is acceptable, at least until there is a better idea.

This is a test for a regression that could occur when splitting date and time arithmetic in Temporal.DateTime.

In order for calendars to work properly, we need to split the Regulate operations into two parts: one to reject or constrain individual values (such as days == 32), which is calendar-sensitive, and one to reject or constrain the entire range of the type (such as -271821-04-18), which is only calendar-sensitive for Date and YearMonth, not for DateTime.

This adds an 'iso8601' calendar and changes the polyfill code for Temporal.Date, Temporal.DateTime, Temporal.YearMonth, and Temporal.MonthDay so that they gain a CALENDAR slot, and delegate all of their calendar-sensitive operations to the object in the CALENDAR slot. Among the changes made to those types: - Constructors gain a calendar argument of type Temporal.Calendar. - Types gain a 'calendar' property of type Temporal.Calendar. - from() accepts a 'calendar' string or Temporal.Calendar. - Temporal.Date and Temporal.DateTime accept a calendar in with(). - Temporal.MonthDay & Temporal.YearMonth throw on a calendar in with(). - The calendar is returned in getFields() (see #641). - equals() and compare() take the calendar into account (see #625). This includes documentation, but leaves out spec changes and all but the necessary test changes for the time being.

Since MonthDay.withYear(year) is not guaranteed to be sufficient to convert to a Temporal.Date if the calendar uses eras as well as years. Instead of picking the arbitrary day 1 or year 2004 to convert, use the refISODay or refISOYear, respectively.

For Intl.DateTimeFormat and toLocaleString(), the Temporal object's calendar should take precedence if it is non-ISO (which here we define as not 'iso8601' or 'gregory'), and otherwise the locale's calendar should take precedence. For ranges, we throw if two Temporal objects are given with differing calendars that are both non-ISO. See: #262

Projects a date or date/time from one calendar into another. Spec for these methods is not written yet.

The exact form of this annotation is under discussion in #293, but this adds the form of it that previously had support in the champions group: [c=identifier] appended to the ISO string. See: #293

This implements "option 1" for the default calendar, which is defaulting to the full ISO 8601 calendar everywhere. We add optional calendar parameters to Temporal.Absolute.prototype.inTimeZone, Temporal.now.date, Temporal.now.dateTime, and Temporal.TimeZone.prototype.getDateTimeFor. See: #292.

In order to allow subclasses to provide their own calendar identifier, even when subclassing ISO8601Calendar rather than Temporal.Calendar directly, we need to make sure the identifier can be passed when chaining up. Adds tests to verify that subclassing Temporal.Calendar in userland works and that a simple object implementing the interface behaves correctly.

The other half of the custom time zones and calendars protocol discussed in #300 was allowing calendars and time zones to be plain objects with the appropriate methods on them. This adds a test verifying that it's possible to use a plain object with calendar methods as a time zone, and makes the changes needed to allow that.

This field is always undefined in the ISO calendar, but is used in several other calendars.

We always need to pass all fields for each type into the calendar methods, in case calendars require more fields. This moves these field accesses into the abstract operations ToTemporalDateRecord, ToTemporalDateTimeRecord, ToTemporalMonthDayRecord, ToTemporalTimeRecord, and ToTemporalYearMonthRecord, since it is otherwise easy to forget a field or forget that they have to be accessed in alphabetical order.

This is identical to the ISO8601 calendar, except for ISO week numbering rules, which we do not currently use in Temporal. See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale/calendar

This is a hacky, untested implementation of the Japanese calendar, intended as a guide for how to implement calendars. It only supports the most recent five eras, and may contain many bugs.

ptomato · 2020-06-10T22:49:35Z

I believe I've addressed the comments now, except for a few that I think should belong in a separate discussion.

What's new: I've also tacked on the hacky gregory and japanese calendars to the end of this pull request but if anyone has concerns about them then I'd rather merge this without them than delay shipping the polyfill any longer.

jasonwilliams · 2020-06-11T16:52:06Z

I don't see any other issues on top of what @sffc has pointed out

sffc

It's hard to tell what changed from my previous review to now, but I trust that you responded to my feedback appropriately, so let's get this in. Thanks!

polyfill/lib/ecmascript.mjs

ptomato · 2020-06-11T23:17:54Z

I plan to merge this first thing tomorrow and then cut a release.

ptomato added this to the Stable API milestone Jun 1, 2020

ptomato requested review from pipobscure, jasonwilliams, sffc, gibson042 and ryzokuken June 1, 2020 19:55

ptomato marked this pull request as draft June 1, 2020 19:55

sffc reviewed Jun 2, 2020

View reviewed changes

ptomato force-pushed the iso-calendar branch from 47cdf3c to b3d6d5a Compare June 3, 2020 00:28

ptomato force-pushed the iso-calendar branch from b3d6d5a to 8947101 Compare June 5, 2020 00:22

ptomato marked this pull request as ready for review June 5, 2020 00:24

ptomato force-pushed the iso-calendar branch from 8947101 to d9eeb83 Compare June 5, 2020 23:12

ptomato marked this pull request as draft June 5, 2020 23:16

ptomato mentioned this pull request Jun 5, 2020

Calendar fixes #650

Merged

ptomato force-pushed the iso-calendar branch from d9eeb83 to 8fcb1a7 Compare June 9, 2020 00:27

ptomato marked this pull request as ready for review June 9, 2020 00:30

ptomato force-pushed the iso-calendar branch from 8fcb1a7 to 9bbe2c6 Compare June 9, 2020 17:17

sffc reviewed Jun 9, 2020

View reviewed changes

sffc requested changes Jun 10, 2020

View reviewed changes

This was referenced Jun 10, 2020

Cookbook example of extra-expanded years #604

Closed

Cookbook example of locked-down Temporal object #603

Open

ptomato mentioned this pull request Jun 10, 2020

Calendar formatter #660

Closed

jasonwilliams reviewed Jun 10, 2020

View reviewed changes

ptomato added 3 commits June 10, 2020 14:30

Test hours overflow in DateTime.plus

67112fb

This is a test for a regression that could occur when splitting date and time arithmetic in Temporal.DateTime.

ptomato added 11 commits June 10, 2020 14:42

Date.withCalendar and DateTime.withCalendar

6af63de

Projects a date or date/time from one calendar into another. Spec for these methods is not written yet.

Serialize calendars with [c=...] annotation in ISO strings

59695b6

The exact form of this annotation is under discussion in #293, but this adds the form of it that previously had support in the champions group: [c=identifier] appended to the ISO string. See: #293

Add 'era' field to Calendar, Date, DateTime, YearMonth

04f38a8

This field is always undefined in the ISO calendar, but is used in several other calendars.

Add Gregorian calendar

b482707

This is identical to the ISO8601 calendar, except for ISO week numbering rules, which we do not currently use in Temporal. See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale/calendar

Add Japanese calendar

915ae53

This is a hacky, untested implementation of the Japanese calendar, intended as a guide for how to implement calendars. It only supports the most recent five eras, and may contain many bugs.

ptomato force-pushed the iso-calendar branch from 9bbe2c6 to 915ae53 Compare June 10, 2020 22:38

ptomato requested review from sffc and jasonwilliams June 10, 2020 22:43

jasonwilliams approved these changes Jun 11, 2020

View reviewed changes

sffc approved these changes Jun 11, 2020

View reviewed changes

polyfill/lib/ecmascript.mjs Show resolved Hide resolved

This was referenced Jun 11, 2020

Should disambiguation apply to the edge of the representable range? #664

Closed

Define legal userland time zone and calendar identifiers #665

Closed

Fields operation on calendar #666

Closed

ptomato merged commit a64fb62 into main Jun 12, 2020

ptomato deleted the iso-calendar branch June 12, 2020 19:05

Ms2ger mentioned this pull request Apr 15, 2022

Polyfill: disallow calendar in Instant strings. #2144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISO 8601 calendar #638

ISO 8601 calendar #638

ptomato commented Jun 1, 2020 •

edited

Loading

sffc Jun 2, 2020

ptomato Jun 2, 2020

sffc commented Jun 2, 2020

ptomato commented Jun 3, 2020

codecov bot commented Jun 5, 2020 •

edited

Loading

sffc left a comment

sffc Jun 9, 2020

ptomato Jun 10, 2020

sffc Jun 9, 2020

ptomato Jun 10, 2020

sffc Jun 11, 2020

ptomato Jun 11, 2020

sffc Jun 12, 2020

sffc left a comment

sffc left a comment

jasonwilliams Jun 10, 2020

ljharb Jun 10, 2020

ptomato Jun 10, 2020

jasonwilliams Jun 11, 2020 •

edited

Loading

jasonwilliams Jun 11, 2020 •

edited

Loading

ljharb Jun 11, 2020

ptomato Jun 11, 2020

ptomato commented Jun 10, 2020

jasonwilliams commented Jun 11, 2020

sffc left a comment

ptomato commented Jun 11, 2020

ISO 8601 calendar #638

ISO 8601 calendar #638

Conversation

ptomato commented Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Jun 2, 2020

ptomato commented Jun 3, 2020

codecov bot commented Jun 5, 2020 • edited Loading

Codecov Report

sffc 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

sffc left a comment

Choose a reason for hiding this comment

sffc 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

jasonwilliams Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

jasonwilliams Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptomato commented Jun 10, 2020

jasonwilliams commented Jun 11, 2020

sffc left a comment

Choose a reason for hiding this comment

ptomato commented Jun 11, 2020

ptomato commented Jun 1, 2020 •

edited

Loading

codecov bot commented Jun 5, 2020 •

edited

Loading

jasonwilliams Jun 11, 2020 •

edited

Loading

jasonwilliams Jun 11, 2020 •

edited

Loading