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

Era and yearType should be extensions defined in 402 #1046

Closed
sffc opened this issue Oct 23, 2020 · 8 comments · Fixed by #1055 or #1195
Closed

Era and yearType should be extensions defined in 402 #1046

sffc opened this issue Oct 23, 2020 · 8 comments · Fixed by #1055 or #1195
Assignees
Labels
non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@sffc
Copy link
Collaborator

sffc commented Oct 23, 2020

262-compliant implementations should not need the .era and .yearType getters on the Date/DateTime/YearMonth prototypes. However, 402-compliant implementations should add those forwarding getters to the prototypes.

@ptomato @littledan

@ptomato ptomato added non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Oct 23, 2020
@ptomato ptomato added this to the Stable proposal milestone Oct 23, 2020
@ptomato
Copy link
Collaborator

ptomato commented Oct 23, 2020

Yes, this needs to be clarified in the spec, and the experimental Japanese calendar implementation in the polyfill needs to add era getters instead of having them pre-defined.

@ptomato ptomato self-assigned this Oct 24, 2020
ptomato added a commit that referenced this issue Oct 24, 2020
This removes era from Temporal proper, and defines it only the first time
a calendar that uses it is constructed. This is in line with era and other
custom calendar properties being the domain of ECMA-402.

Closes: #1046
ptomato added a commit that referenced this issue Oct 26, 2020
This removes era from Temporal proper, and defines it only the first time
a calendar that uses it is constructed. This is in line with era and other
custom calendar properties being the domain of ECMA-402.

Closes: #1046
ptomato added a commit that referenced this issue Oct 27, 2020
This removes era from Temporal proper, and defines it only the first time
a calendar that uses it is constructed. This is in line with era and other
custom calendar properties being the domain of ECMA-402.

Closes: #1046
Ms2ger pushed a commit that referenced this issue Oct 28, 2020
This removes era from Temporal proper, and defines it only the first time
a calendar that uses it is constructed. This is in line with era and other
custom calendar properties being the domain of ECMA-402.

Closes: #1046
@littledan
Copy link
Member

I'm not really a fan of this change: it adds own getters, diverging from the typical pattern of having all getters on the prototype. (As a more minor point, it creates a divergent "shape" for various instances of PlainDate, PlainDateTime, PlainZonedDateTime and PlainYearMonth, so accesses could be "more polymorphic" and costly if mixed calendars are used, even for parts of the accesses that are not calendar-based.)

I'm having trouble understanding why the 262/402 split is relevant.

I'd suggest returning previous approach, where era was always defined.

(This is a small detail, so I think we should work it out by Stage 3, but I don't think it blocks the "stable proposal" milestone.)

@littledan littledan reopened this Nov 13, 2020
@sffc
Copy link
Collaborator Author

sffc commented Nov 13, 2020

The problem isn't about era. It's about the fact that the additional fields are inherently calendar-specific. In addition to era, we need other fields such as:

  • yearType for the Hebrew calendar
  • isLeapMonth for the Chinese calendar
  • Possibly other fields for the traditional Islamic calendar

The set of fields squarely lands in 402's territory. I feel strongly that 262 should not define fields beyond those needed for the ISO-8601 calendar.

@littledan
Copy link
Member

littledan commented Nov 13, 2020

Sorry, it seems like I misread the polyfill. There are no own getters. Instead, the prototype getters are defined once an instance of the Japanese calendar is created.

I think we could define additional properties of Date types in 402, but I think they should be present from when the Realm is created, not when a calendar instance is created.

EDIT: Looks like you said the same thing in #1099 (comment) so I guess we're in agreement.

@littledan
Copy link
Member

littledan commented Nov 13, 2020

My recommended action is to revert #1055, as explained at #1055 (comment) . Instead, the solution to this issue is editorial, where we separate out 262 and 402 sections in the specification.

@justingrant
Copy link
Collaborator

it creates a divergent "shape" for various instances of PlainDate, PlainDateTime, PlainZonedDateTime [sic] and PlainYearMonth

FWIW, this is a problem regardless of the implementation. The challenge here is that it's not just era. It's a potentially wide variety of calendar-specific fields that don't exist in other calendars.

It seems like bad DX to have a bunch of undefined-valued fields on all instances of most Temporal types (PlainDate, PlainDateTime, ZonedDateTime, PlainMonthDay, PlainYearMonth, and even PlainTime) in the vast-majority case where only ISO is being used. And are there possible perf-related impact too, especially if the number of "extra" fields is large, e.g. JSON serialization or spreading of getFields() with 10 extra undefined properties coming along for the ride for every Temporal object using the ISO calendar .

Is there any way around this problem? Could/should getFields() omit fields that have an undefined value?

@sffc
Copy link
Collaborator Author

sffc commented Nov 13, 2020

This is only for prototype getters. Undefined fields should never be returned in getFields(). The calendar decides which subset of fields to include in getFields().

I do agree that the era/yearType/isLeapMonth getters should be on the prototype when the realm is created if the realm contains 402. Custom calendars can decide how and when to add them, but for built-in 402 calendars, it seems like the properties should be on the prototype from startup.

@justingrant
Copy link
Collaborator

This is only for prototype getters. Undefined fields should never be returned in getFields().

OK, sounds reasonable to me.

ptomato added a commit that referenced this issue Nov 15, 2020
This partially reverts commit a7322a6.
'era' is still not present in the spec text, but is present in the
polyfill since this environment includes Intl.

Closes: #1046
ptomato added a commit that referenced this issue Nov 16, 2020
This partially reverts commit a7322a6.
'era' is still not present in the spec text, but is present in the
polyfill since this environment includes Intl.

Closes: #1046
ptomato added a commit that referenced this issue Nov 16, 2020
This partially reverts commit a7322a6.
'era' is still not present in the spec text, but is present in the
polyfill since this environment includes Intl.

Closes: #1046
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
4 participants