Polyfill: fix a few non-ISO calendar issues #1977

justingrant · 2021-12-13T11:13:08Z

Fix a few non-ISO calendar issues:

Update documentation comment for era metadata d458938 (the current in-code documentation for internal-only era definitions are out of date)
Call maximumMonthLength with correct args efe4498
Additional validation for hard-coded era data 592b100
Fix bad code ordering in adjustCalendarDate ba96615

This PR used to have a larger scope but @ptomato helpfully split some of the easier ones into #1983.

codecov · 2021-12-13T11:14:11Z

Codecov Report

Merging #1977 (ba96615) into main (750c7cc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1977   +/-   ##
=======================================
  Coverage   94.96%   94.96%           
=======================================
  Files          19       19           
  Lines       10962    10967    +5     
  Branches     1742     1743    +1     
=======================================
+ Hits        10410    10415    +5     
  Misses        535      535           
  Partials       17       17

Flag	Coverage Δ
test262	`79.29% <22.22%> (-0.04%)`	⬇️
tests	`89.86% <96.42%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
polyfill/lib/calendar.mjs	`94.31% <100.00%> (+0.01%)`	⬆️

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 750c7cc...ba96615. Read the comment docs.

Ms2ger

I'm happy to land the cached built-in objects, let → const and unused parameter changes. I need to think more carefully or at least hear some more justification for the rest.

polyfill/lib/calendar.mjs

Ms2ger · 2021-12-13T13:23:48Z

polyfill/lib/calendar.mjs

@@ -633,18 +633,20 @@ const nonIsoHelperBase = {
  adjustCalendarDate(calendarDate, cache, overflow /*, fromLegacyDate = false */) {
    if (this.calendarType === 'lunisolar') throw new RangeError('Override required for lunisolar calendars');
    this.validateCalendarDate(calendarDate);
-    const largestMonth = this.monthsInYear(calendarDate, cache);
-    let { month, year, eraYear, monthCode } = calendarDate;


Are you sure this isn't intentionally controlling the order of calling any getters on calendarDate?

Yep. calendarDate here is a record, not a Temporal object. The only access to Temporal instances is via slots. For the non-ISO calendar implementation, the only slot access happens in temporalToCalendarDate(). So there shouldn't be any observable property access at all. The naming convention in calendar.mjs is this:

"temporalDate" - temporal instance

"isoDate" - { year: number; month: number; day: number } record representing a date in the ISO calendar

"calendarDate" - record with properties representing the values of built-in calendar fields, with a shape that varies based on the method. For this method, the shape of the calendarDate param has this type over in temporal-polyfill:

{ era?: string | undefined; eraYear?: number | undefined; year?: number | undefined;; month?: number | undefined;; monthCode?: string | undefined;; day?: number | undefined;; monthExtra?: string | undefined; // used internally; never from external data }

Does that address your concerns?

with a shape that varies based on the method

More specifically, all "calendarDate" objects have the shape above, but depending on the method and where the input comes from, some properties may be missing vs. present. For example, if the input to the method commented above is coming from user calls, then it will match the input of Calendar.dateFromFields. If the input is internally generated, it might only be YMD fields. Other methods may only care about some subset of the fields. For example, the inLeapYear methods of these *Helper objects only care about the year property and ignores the rest.

The TS polyfill (especially in js-temporal/temporal-polyfill#109 makes this all a lot clearer because everything is now typed.

polyfill/lib/calendar.mjs

justingrant

I'm happy to land the cached built-in objects, let → const and unused parameter changes. I need to think more carefully or at least hear some more justification for the rest.

Sounds good. I replied to your comments above and added comments to explain the purpose of anything other than "cached built-in objects, let → const and unused parameter changes".

polyfill/lib/calendar.mjs

justingrant · 2021-12-13T16:46:02Z

polyfill/lib/calendar.mjs

@@ -1146,10 +1148,10 @@ const helperHebrew = ObjectAssign({}, nonIsoHelperBase, {
      } else {
        if (overflow === 'reject') {
          ES.RejectToRange(month, 1, this.monthsInYear({ year }));
-          ES.RejectToRange(day, 1, this.maximumMonthLength(calendarDate));
+          ES.RejectToRange(day, 1, this.maximumMonthLength({ year, month }));


This was a legit bug fix. { year, month } might have been mutated since they were extracted from calendarDate. I wasn't able to figure out an externally-visible repro case that this bug broke, but it's definitely fragile to changes elsewhere and seemed worth fixing here too.

@jens-ox

This PR focuses on unblocking `strictNullChecks` and `strictPropertyInitialization` in ecmascript.ts and calendar.ts. It's intended as a complement to @jens-ox's work to remove `GetIntrinsic`. This PR: * Enables strictNullChecks: true and strictPropertyInitialization: true in tsconfig.json. * Improves calendar.ts TS types (this was most of the work in this PR) * Removes all unnecessary use of `any` throughout the polyfill * Updates types of ecmascript.ts and fixes a number of type bugs * Ports several proposal-temporal PRs: * tc39/proposal-temporal#1975 - Refactors to align ecmascript.mjs with spec text * tc39/proposal-temporal#1974 - Ensure that Temporal prototypes aren't writable * tc39/proposal-temporal#1976 - Throw RangeError if there's an invalid offset string in ZonedDateTime-representing property bags * tc39/proposal-temporal#1977 - Refactor and fix non-ISO calendars in polyfill * Fixes a few index.d.ts types * Refactors a few types in intl.ts * A handful of trivial type changes in other files (ecmascript.ts and calendar-ts were 95%+ of the work). I'd like to split out the runtime behavior changes (mostly the ported PRs above) into a smaller PR which should make it easier to review changes that can actually break things at runtime. I also need to port tests over from tc39/proposal-temporal#1976. Once those are split out, this PR should be ready to review.

justingrant · 2021-12-15T16:29:59Z

@ptomato @Ms2ger Sorry to bug you guys but would love a review on this so I can unblock the next stage of getting js-temporal/temporal-polyfill#109 landed. (That PR depends on porting a bunch of proposal-temporal PRs to temporal-polyfill, and this PR here is the last one needed to be approved from the big batch this week.)

ptomato · 2021-12-15T18:51:56Z

@justingrant Unfortunately it'll be fairly difficult for me to get thoroughly into a pull request this large before leaving on end-of-year vacation. What I maybe can do, is try to split it up into different commits, and merge the things that Ms2ger already OK'd. Maybe that would unblock some of the changes that are waiting on this? It should also make the remaining code easier to review, as well.

justingrant · 2021-12-15T19:04:35Z

OK sounds good, I'll split it.

ptomato · 2021-12-15T20:35:19Z

I'm happy to do the splitting this afternoon, or tomorrow if I don't get to it today.

justingrant · 2021-12-15T21:29:58Z

I'm happy to do the splitting this afternoon, or tomorrow if I don't get to it today.

Nah, I can do the splitting to save scarce @ptomato time for reviewing PRs like js-temporal/temporal-polyfill#111. 😄

ptomato · 2021-12-15T21:45:03Z

Splitting commits and rubber-stamping backports is something that doesn't require me to concentrate, so it's all good 😄 — I opened #1983 but I'll leave the rest of the splitting to you in that case.

`year` might have been mutated in the first line of this method. So it's not OK to pass `calendarDate` to `maximumMonthLength`. Need to pass current year/month values instead.

This adds an additional check to make sure that the (hard-coded) era definitions for each calendar aren't missing required `isoEpoch` properties. Note that this code is only run once at load time, not each time calendars are accessed, so there's no runtime impact.

Re-order code to guarantee that `calendarDate.year` is set before calculating the number of months in that year. The old code would break for any calendar that a) used a constant `era` like `islamic` and b) had a variable number of months in each year like `hebrew`. Today no ICU calendars fit in that Venn diagram, but if any are added in the future this code will break when initializing any calendar using an eraYear/era pair in a property bag. Better to fix it now!

justingrant · 2021-12-15T23:24:39Z

@ptomato I split out the remaining edits into 4 separate commits with comments explaining why they're there. Thanks!

This was originally part of tc39/proposal-temporal#1977 but I removed it because it wasn't high-priority for JS. But it's still needed in this repo so I'm adding it back. Why? TS complains when you assign a maybe-undefined value to a must-not-be-undefined variable. This small runtime change is cleaner than adding a lotta type casts.

ptomato

Thanks, much easier to digest now.

This was originally part of tc39/proposal-temporal#1977 but I removed it because it wasn't high-priority for JS. But it's still needed in this repo so I'm adding it back. Why? TS complains when you assign a maybe-undefined value to a must-not-be-undefined variable. This small runtime change is cleaner than adding a lotta type casts.

justingrant requested review from Ms2ger and ptomato December 13, 2021 11:13

justingrant mentioned this pull request Dec 13, 2021

Compile with strictNullChecks/strictPropertyInitialization (focus on calendar.ts and ecmascript.ts) js-temporal/temporal-polyfill#109

Merged

Ms2ger reviewed Dec 13, 2021

View reviewed changes

justingrant commented Dec 13, 2021

View reviewed changes

polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved

justingrant commented Dec 13, 2021

View reviewed changes

justingrant requested a review from Ms2ger December 15, 2021 16:22

justingrant added no-spec-text PR can be ignored by implementors non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! labels Dec 15, 2021

ptomato mentioned this pull request Dec 15, 2021

Non ISO calendar refactor, part 1 #1983

Merged

justingrant added 4 commits December 15, 2021 15:14

Update documentation comment for era metadata

d458938

Call maximumMonthLength with correct args

efe4498

`year` might have been mutated in the first line of this method. So it's not OK to pass `calendarDate` to `maximumMonthLength`. Need to pass current year/month values instead.

justingrant force-pushed the non-iso-calendar-refactor branch from dd05325 to ba96615 Compare December 15, 2021 23:16

justingrant changed the title ~~Refactor and fix non-ISO calendars in polyfill~~ Polyfill: fix a few non-ISO calendar issues Dec 15, 2021

justingrant mentioned this pull request Dec 16, 2021

Port non-ISO calendar fixes from proposal-temporal PRs #1977 and #1983 js-temporal/temporal-polyfill#112

Merged

ptomato approved these changes Dec 16, 2021

View reviewed changes

ptomato merged commit 82cbc94 into tc39:main Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polyfill: fix a few non-ISO calendar issues #1977

Polyfill: fix a few non-ISO calendar issues #1977

justingrant commented Dec 13, 2021 •

edited

Loading

codecov bot commented Dec 13, 2021 •

edited

Loading

Ms2ger left a comment

Ms2ger Dec 13, 2021

justingrant Dec 13, 2021 •

edited

Loading

justingrant Dec 13, 2021

justingrant left a comment

justingrant Dec 13, 2021

justingrant commented Dec 15, 2021

ptomato commented Dec 15, 2021

justingrant commented Dec 15, 2021

ptomato commented Dec 15, 2021

justingrant commented Dec 15, 2021

ptomato commented Dec 15, 2021

justingrant commented Dec 15, 2021

ptomato left a comment

Polyfill: fix a few non-ISO calendar issues #1977

Polyfill: fix a few non-ISO calendar issues #1977

Conversation

justingrant commented Dec 13, 2021 • edited Loading

codecov bot commented Dec 13, 2021 • edited Loading

Codecov Report

Ms2ger left a comment

Choose a reason for hiding this comment

Ms2ger Dec 13, 2021

Choose a reason for hiding this comment

justingrant Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

justingrant Dec 13, 2021

Choose a reason for hiding this comment

justingrant left a comment

Choose a reason for hiding this comment

justingrant Dec 13, 2021

Choose a reason for hiding this comment

justingrant commented Dec 15, 2021

ptomato commented Dec 15, 2021

justingrant commented Dec 15, 2021

ptomato commented Dec 15, 2021

justingrant commented Dec 15, 2021

ptomato commented Dec 15, 2021

justingrant commented Dec 15, 2021

ptomato left a comment

Choose a reason for hiding this comment

justingrant commented Dec 13, 2021 •

edited

Loading

codecov bot commented Dec 13, 2021 •

edited

Loading

justingrant Dec 13, 2021 •

edited

Loading