Skip to content
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.

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

weekOfYear / yearOfWeek is not clearly specified for non-ISO calendars #2744

Closed
justingrant opened this issue Dec 22, 2023 · 6 comments · Fixed by #2756
Closed

weekOfYear / yearOfWeek is not clearly specified for non-ISO calendars #2744

justingrant opened this issue Dec 22, 2023 · 6 comments · Fixed by #2756
Assignees
Labels
calendar Part of the effort for Temporal Calendar API non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@justingrant
Copy link
Collaborator

justingrant commented Dec 22, 2023

As part of investigating #2740, I noticed that weekOfYear and yearOfWeek implementations in the polyfill all use the ISO calendar, meaning that they always return the ISO year, or one more or less than the ISO year. This is incorrect based on the spec text excerpted below.

But the spec is also silent about how non-ISO calendars should implement these properties, specifically how to handle the weeks at start or end of year. This seems like an open invitation for implementations to diverge.

Should we add text that explains how to calculate these values? Should we require all calendars to use the same algorithm used for ISO? Should these methods throw for non-ISO calendars, because the whole point of these methods is to expose the ISO calendar's week-numbering algorithm?

15.8.1.15 CalendarDateWeekOfYear ( calendar, date )
The abstract operation CalendarDateWeekOfYear takes arguments calendar (a String) and date (a Temporal.PlainDateTime or Temporal.PlainDate) and returns a Year-Week Record. It takes a date-like object date and calculates the calendar week of year, and the corresponding week calendar year, in the calendar represented by calendar.

The value in the [[Week]] field should be 1-based.

The value in the [[Year]] field is relative to the first day of a calendar-specific "epoch year", as in CalendarDateYear, not relative to an era as in CalendarDateEraYear.

Usually the [[Year]] field will contain the same value given by CalendarDateYear, but may contain the previous or next year if the week number in the [[Week]] field overlaps two different years. See also ToISOWeekOfYear.

@justingrant justingrant added non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! calendar Part of the effort for Temporal Calendar API labels Dec 22, 2023
@MohsenAlyafei
Copy link

@justingrant This an interesting topic and needs to be thought of carefully to avoid implementation divergence.

The current ISO week year standard is based on the Gregory calendar and there are no ISO standards for other calendars.

As Temporal is in its latest stages with implementation on the horizon, it would be safer for the time being to throw an error for non Gregory calendars until such time a sound and permanent fix is developed.
The 2 properties may possibly for now return 'undefined' with future revision introducing solutions per calendar.

The concept of introducing the same algorithm as used by ISO for other calendar into Temporal sounds great and will require further discussion. Under such solution, for Islamic calendars, the first day of the week is Saturday which is the first day after the holy rest day of Friday. The Islamic world considers Saturday as first day of the week.

So an ISO Hijri (Islamic) week calendar-like will always start on Saturday and the middle day of the week is Tuesday. This follows the same concept of the current ISO week calendar based on Gregory calendar which start on Monday, with Thursday being the middle day.

@MohsenAlyafei
Copy link

As a follow up on my post above regarding the last 2 paragraphs regarding implementation of a Hijri ISO-like week calendar, this does not mean that the existing week day numbering system should be changed for a Hijri calendar.

I.e. day 1 is Monday and such numbering should remain as it is only a reference number. This is similar to the concept of when a work day starts which could different in different countries and does not impact day numbering.

@justingrant
Copy link
Collaborator Author

day 1 is Monday and such numbering should remain as it is only a reference number. This is similar to the concept of when a work day starts which could different in different countries and does not impact day numbering.

This is exactly correct. Temporal is locale-neutral; its focus is on software calculations like determining the number of years or months or days between dates, adding a duration to a date, or ensuring that calculations work as expected across time zones and calendars. Any calculation that depends on knowing the locale (for example, determining the first day of the week) happens in Intl, not Temporal.

I find it useful to think about two subtly different concepts involved with days of the week: *indexing*\ of days (e.g. 1 means Monday) and *determining the first day of the week*\ which is locale-sensitive, as nicely summarized by Wikipedia:

Most of Europe and China consider Monday the first day of the week, most of North America and South Asia consider Sunday the first day, while Saturday is judged as the first day of the week in much of the Middle East and North Africa.

In ECMAScript, the calendar determines indexing (Temporal uses calendars' indexes as opaque indexes that don't have semantic meaning), while it's
Intl.Locale.prototype.getWeekInfo() that helps developers determine which index represents the start of the week, the weekend, etc. For example, if you want to calculate how many weeks are between two dates, you'd use Temporal. But if you want to build a calendar UI, then you'll need to know the Locale info too.

It's an interesting question of whether different calendars may want to use different indexes, for example 5 could mean Monday. FWIW, the gregory calendar chose to not to diverge from the ISO weekday indexes, even though in many countries like the USA, Sunday not Monday is the first day of the week.

I don't have an opinion about how other calendars should design this, but I think you're correct that treating the dayOfWeek in Temporal as a "reference number" (not a semantically meaningful thing) has some advantages.

cc @sffc you may be interested in this thread.

@sffc
Copy link
Collaborator

sffc commented Dec 27, 2023

Not opposed to disabling these functions in non-ISO calendars.

I think the case of the Islamic calendar is particularly interesting because the calendar system carries context on how weeks work. I wouldn't want to box us into a particular behavior until we think it through.

Alternatively we can consider this to be in scope for https://github.com/tc39/proposal-intl-era-monthcode

@gibson042
Copy link
Collaborator

gibson042 commented Dec 28, 2023

Should we add text that explains how to calculate these values? Should we require all calendars to use the same algorithm used for ISO? Should these methods throw for non-ISO calendars, because the whole point of these methods is to expose the ISO calendar's week-numbering algorithm?

I don't think it makes sense to require an analogue of the ISO 8601 algorithm, because there very well might be other appropriate behavior for a particular calendar. Throwing is probably best, with second-best being indication by otherwise-invalid values such as undefined or NaN. However, note that a normative change is required to take either approach, because right now CalendarDateWeekOfYear is specified to return a Year-Week Record in which both [[Year]] and [[Week]] fields are integer-valued, and both Temporal.Calendar.prototype.weekOfYear and Temporal.Calendar.prototype.yearOfWeek rely upon that operation to do so with no calendar-sensitive escape hatch.

So I think we want to

  1. update CalendarDateWeekOfYear to support returning a throw completion and recommend such a return for any calendar without a well-defined week calendar system, and

  2. possibly also change Year-Week Record to support NaN values (in anticipation of e.g. special-case intercalary dates), and

  3. update the weekOfYear and yearOfWeek algorithms accordingly:

                 1. Let _temporalDate_ be ? ToTemporalDate(_temporalDateLike_).
                 1. If _calendar_.[[Identifier]] is *"iso8601"*, then
                   1. Let _yearWeek_ be ToISOWeekOfYear(_temporalDate_.[[ISOYear]], _temporalDate_.[[ISOMonth]], _temporalDate_.[[ISODay]]).
                 1. Else,
    -              1. Let _yearWeek_ be CalendarDateWeekOfYear(_calendar_.[[Identifier]], _temporalDate_).
    +              1. Let _yearWeek_ be ? CalendarDateWeekOfYear(_calendar_.[[Identifier]], _temporalDate_).
                 1. Return 𝔽(_yearWeek_.[[Year]]).

@ptomato ptomato added spec-text Specification text involved normative Would be a normative change to the proposal and removed meeting-agenda labels Jan 11, 2024
@ptomato ptomato self-assigned this Jan 11, 2024
@ptomato ptomato added this to the Stage "3.5" milestone Jan 11, 2024
@ptomato
Copy link
Collaborator

ptomato commented Jan 11, 2024

We talked about this in today's champions meeting. There is precedent for this: CalendarDateEra gives undefined if the calendar doesn't have eras. I'll prepare a change for this, that we can present in the February plenary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
calendar Part of the effort for Temporal Calendar API non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants