Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 16, 2021
Merged

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d458938
Update documentation comment for era metadata
justingrant Dec 15, 2021
efe4498
Call maximumMonthLength with correct args
justingrant Dec 15, 2021
592b100
Additonal validation for hard-coded era data
justingrant Dec 15, 2021
ba96615
Fix bad code ordering in `adjustCalendarDate`
justingrant Dec 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 28 additions & 23 deletions polyfill/lib/calendar.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@justingrant justingrant Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// For calendars that always use the same era, set it here so that derived
// calendars won't need to implement this method simply to set the era.
if (this.constantEra) {
// year and eraYear always match when there's only one possible era
if (year === undefined) year = eraYear;
if (eraYear === undefined) eraYear = year;
calendarDate = { ...calendarDate, era: this.constantEra, year, eraYear };
const { year, eraYear } = calendarDate;
calendarDate = {
...calendarDate,
era: this.constantEra,
year: year !== undefined ? year : eraYear,
eraYear: eraYear !== undefined ? eraYear : year
};
}

const largestMonth = this.monthsInYear(calendarDate, cache);
let { month, monthCode } = calendarDate;
({ month, monthCode } = resolveNonLunisolarMonth(calendarDate, overflow, largestMonth));
return { ...calendarDate, month, monthCode };
},
Expand Down Expand Up @@ -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 }));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
month = ES.ConstrainToRange(month, 1, this.monthsInYear({ year }));
day = ES.ConstrainToRange(day, 1, this.maximumMonthLength({ ...calendarDate, month }));
day = ES.ConstrainToRange(day, 1, this.maximumMonthLength({ year, month }));
}
if (monthCode === undefined) {
monthCode = this.getMonthCode(year, month);
Expand Down Expand Up @@ -1298,36 +1300,38 @@ const helperIndian = ObjectAssign({}, nonIsoHelperBase, {
* eras. Note that it mutates and normalizes the original era objects, which is
* OK because this is non-observable, internal-only metadata.
*
* The result is an array of eras with this shape:
* ```
* interface Era {
* // name of the era
* interface Era {
* /** name of the era
* name: string;
*
* // alternate name of the era used in old versions of ICU data
* // format is `era{n}` where n is the zero-based index of the era
* // with the oldest era being 0.
* genericName: string;
*
* // Signed calendar year where this era begins. Will be
* // 1 (or 0 for zero-based eras) for the anchor era assuming that `year`
* // numbering starts at the beginning of the anchor era, which is true
* // for all ICU calendars except Japanese. If an era starts mid-year
* // then a calendar month and day are included. Otherwise
* // `{ month: 1, day: 1 }` is assumed.
* anchorEpoch: { year: number } | { year: number, month: number, day: number }
* // Signed calendar year where this era begins. Will be 1 (or 0 for zero-based
* // eras) for the anchor era assuming that `year` numbering starts at the
* // beginning of the anchor era, which is true for all ICU calendars except
* // Japanese. For input, the month and day are optional. If an era starts
* // mid-year then a calendar month and day are included.
* // Otherwise `{ month: 1, day: 1 }` is assumed.
* anchorEpoch: { year: number; month: number; day: number };
*
* // ISO date of the first day of this era
* isoEpoch: { year: number, month: number, day: number}
* isoEpoch: { year: number; month: number; day: number };
*
* // If present, then this era counts years backwards like BC
* // and this property points to the forward era. This must be
* // the last (oldest) era in the array.
* reverseOf: Era;
* reverseOf?: Era;
*
* // If true, the era's years are 0-based. If omitted or false,
* // then the era's years are 1-based.
* hasYearZero: boolean = false;
* hasYearZero?: boolean;
*
* // Override if this era is the anchor. Not normally used because
* // anchor eras are inferred.
* isAnchor?: boolean;
* }
* ```
* */
Expand Down Expand Up @@ -1387,6 +1391,7 @@ function adjustEras(eras) {
ArraySort.call(eras, (e1, e2) => {
if (e1.reverseOf) return 1;
if (e2.reverseOf) return -1;
if (!e1.isoEpoch || !e2.isoEpoch) throw new RangeError('Invalid era data: missing ISO epoch');
return e2.isoEpoch.year - e1.isoEpoch.year;
});

Expand Down