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 isn't implemented in ISO calendar. Expected? #901

Closed
justingrant opened this issue Sep 15, 2020 · 19 comments · Fixed by #1048
Closed

era isn't implemented in ISO calendar. Expected? #901

justingrant opened this issue Sep 15, 2020 · 19 comments · Fixed by #1048
Assignees
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! question spec-text Specification text involved

Comments

@justingrant
Copy link
Collaborator

Are we planning to implement the era property on the ISO calendar?

Temporal.Absolute.from('2020-08-05T20:06:13+0900').toDateTime('Europe/Paris').era

Expected: "AD"? "CE"? "A.D."? "C.E."?
Actual: undefined

I can see the justification for not implementing it (e.g. if we don't want to wade into the political debate about AD vs. CE) but if it's intentionally not implemented then should the property throw when called?

@ptomato
Copy link
Collaborator

ptomato commented Sep 15, 2020

era is supported as a formatting parameter for the Gregorian calendar in the Intl.DateTimeFormat because some date formats need it, but as a component of the date for Temporal it's redundant in the Gregorian calendar and I believe we had already decided not to implement it. (I can't quite remember where the discussion took place.) In Intl.DateTimeFormat it'll work just as it did with legacy Date.

But it should definitely not throw, its value is just undefined on dates in calendars where it doesn't have a meaning.

@justingrant
Copy link
Collaborator Author

What about this?

Temporal.Absolute.from('2020-08-05T20:06:13+0900').toDateTime('Europe/Paris').with({era: 'BC'}).era

Seems really odd that I can "set" a property using with (without an exception) but the value is not actually used and is not available when I read it back.

IMHO, if era is only readable or settable in a minority of calendars, it feels more appropriate to make era into a field that's added by non-ISO calendars but isn't part of the base set of fields available on all calendars.

Doing this would probably be helpful to validate Temporal's end-to-end support for calendars adding additional properties, including:

  • How to expose to TypeScript? I have an idea how to do this, which is to make Temporal types into generic types that take the calendar type as a generic type parameter (e.g. interface Date<C extends Calendar = ISO8601Calendar>) that would work for each built-in calendar, but this idea would need to be proven out to make sure it'd work.
  • Setting default values for extra fields (and non-extra fields like hour: 6 for ethiopic).
  • Handling required extra fields, e.g. throwing if era is required but not provided
  • Conversions, e.g. does withCalendar strip out fields not used in the destination calendar?
  • Constructing instances with extra fields. For example, how to add extra fields to DateTime constructor args?

I'm not familiar enough with calendar plans to know if solutions to all of above are already defined and tested. But seems like removing era from the base type would be a good way to ensure that things actually work as we'd expect.

@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

We had agreed elsewhere (can't remember where) that era ought to be present on the base type, since it's used in many built-in calendars, but that it should return undefined for the ISO calendar and other calendars that don't use it. @sffc do you remember?

@ptomato ptomato added this to the Stable proposal milestone Sep 17, 2020
@sffc
Copy link
Collaborator

sffc commented Sep 17, 2020

ICU is wishy-washy here. It always has an era available, but sometimes it is bogus, since not all calendars have any concept of era. I think ICU uses AD/BC for Gregorian. I would have a slight preference for Temporal to keep era as a property only on calendars that actually need it in their data model.

Seems really odd that I can "set" a property using with (without an exception) but the value is not actually used and is not available when I read it back.

That's the case with literally any property passed to with except for the very small set the calendar understands. You could pass .with({ year: 2020, foo: "bar" }) and foo would be ignored.

IMHO, if era is only readable or settable in a minority of calendars, it feels more appropriate to make era into a field that's added by non-ISO calendars but isn't part of the base set of fields available on all calendars.

This would be possible if we made all calendar fields into own properties (#917). Otherwise, it would be even more weird if Japanese dates had an own property but Gregorian dates didn't have any own properties. Putting the property on the prototype puts things on an even playing field. This is why things are the way they are right now.

@sffc
Copy link
Collaborator

sffc commented Sep 17, 2020

Also see #918 for another potential solution.

@justingrant
Copy link
Collaborator Author

That's the case with literally any property passed to with except for the very small set the calendar understands. You could pass .with({ year: 2020, foo: "bar" }) and foo would be ignored.

Sure, but we don't return foo: undefined as a property in getFields(). And we don't show foo as a property in the TS types for Date and DateTime.

My main issue is that we're implying (via getFields, .d.ts, docs, etc) that era is a real property on types where era is never used and is meaningless. IMHO we should either make into a real field that has a real value, or we should make it only available on calendars that use it. Where we are now feels like an uncanny valley that I'd like to avoid.

That said, I realize that there are really separate questions here:

  1. Should the ISO calendar expose a value for era? If yes, then what should be its value?
  2. Should passing era into with() of an ISO-calendar object have any impact on the output? If yes, then what values are accepted?
    (The questions below assume that the answer to both 1 and 2 are "no")
  3. Should era be exposed in getFields for types where it's not used?
  4. Should era be exposed in TS types for types where it's not used?
  5. Should custom calendar implementers be allowed to add custom calendar-specific properties? Should this be prevented? If not, what guidance do we have for them in how to implement this?
  6. What is our plan if we discover that one or more built-in calendars need additional calendar-specific properties beyond era?
  7. Should era be documented as a property in the docs for Date, DateTime, etc.? Or should it be described in a separate section, e.g. "Calendar-specific properties" that makes it clear that the property is coming from the Calendar, not from Date/DateTime itself?
  8. Which calendars actually use this property? Are there others besides Japanese where it's required?

Putting the property on the prototype puts things on an even playing field. This is why things are the way they are right now.

Are we worried that this just kicks the can down the road to the first calendar that wants to add its own properties? Is there something special about "era" that won't apply to the next non-ISO property we discover?

@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

  1. Yes, "undefined"
  2. No (and I'd say that even in the Japanese calendar someDate.with({ era: 'heisei' }) is arbitrary and unlikely to be a use case)
  3. No strong opinion, but we had previously decided yes
  4. No opinion
  5. Yes, guidance should be in the cookbook, but is not written yet. We have a paragraph on this (see below the "Calendar interface" in https://tc39.es/proposal-temporal/docs/calendar-draft.html#methods-on-the-temporalcalendar-interface)
  6. No strong opinion, but I'd guess they should be treated the same as era. An example might be yearType for the Hebrew calendar though that is different because it's a characteristic of the date (like daysInYear) rather than a required field of the date (like era for the Japanese calendar).
  7. It should be documented as a property of Temporal.Date and the other types.
  8. I don't know off the top of my head, but I seem to remember there are.

Is there something special about "era" that won't apply to the next non-ISO property we discover?

I think it's more accurate to say there's something special about era that won't apply to the next non-ISO, userland calendar property we discover.

@ptomato ptomato added documentation Additions to documentation spec-text Specification text involved non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! labels Sep 18, 2020
@sffc
Copy link
Collaborator

sffc commented Sep 18, 2020

That's the case with literally any property passed to with except for the very small set the calendar understands. You could pass .with({ year: 2020, foo: "bar" }) and foo would be ignored.

Sure, but we don't return foo: undefined as a property in getFields().

Oh, we definitely should not return era in getFields() for the ISO calendar. I don't think we do. It's just that we have a function on the prototype that proxies to the calendar.

And we don't show foo as a property in the TS types for Date and DateTime.

I claim it's only a problem for TypeScript, then. For regular JavaScript, a non-existent property is virtually indistinguishable from a property that returns undefined. You should be able to do some TypeScript shenanigans to fix the TypeScript problem.

For your questions:

  1. Should the ISO calendar expose a value for era? If yes, then what should be its value?
  2. Should passing era into with() of an ISO-calendar object have any impact on the output? If yes, then what values are accepted?

No and no.

  1. Should era be exposed in getFields for types where it's not used?

Absolutely not.

  1. Should era be exposed in TS types for types where it's not used?

Semantically, no. As I said above, I believe this is a TypeScript problem, not a Temporal problem.

  1. Should custom calendar implementers be allowed to add custom calendar-specific properties? Should this be prevented? If not, what guidance do we have for them in how to implement this?

Yes, they should. The imperfect way we were planning on doing this was to include enough properties by default to cover the 402 calendars, and most custom calendars should fit inside that set of properties, and if they want more properties, they extend the Temporal.Date prototype. Yuck, I know.

  1. What is our plan if we discover that one or more built-in calendars need additional calendar-specific properties beyond era?

In the current model, my understanding is that all properties beyond those required for the ISO calendar should be extensions to the Temporal prototypes defined in 402 (rather than 262). So, 402 is free to add whatever it wants to the prototype.

  1. Should era be documented as a property in the docs for Date, DateTime, etc.? Or should it be described in a separate section, e.g. "Calendar-specific properties" that makes it clear that the property is coming from the Calendar, not from Date/DateTime itself?

era is in the same category as year, month, and day, and it should be documented in the same way. All of those properties return data calculated by or proxied through the calendar. Hypothetically, a calendar could not define months or years at all, and that's okay.

  1. Which calendars actually use this property? Are there others besides Japanese where it's required?

Yes, there are, but I'd need to look it up to get a full list.

Putting the property on the prototype puts things on an even playing field. This is why things are the way they are right now.

Are we worried that this just kicks the can down the road to the first calendar that wants to add its own properties? Is there something special about "era" that won't apply to the next non-ISO property we discover?

No, there's nothing special about "era". It should not be in 262. It should, however, be in 402, and TypeScript should not complain if you try accessing it.

@justingrant
Copy link
Collaborator Author

justingrant commented Sep 18, 2020

@sffc @ptomato Could we discuss this in tomorrow's meeting or a subsequent one?

  1. Should era be exposed in getFields for types where it's not used?

@sffc Absolutely not.

I agree with Shane here. This one is probably the top priority in my list in this issue. IMHO, types should not return fields in the getFields result that are not used by that calendar. For the ISO calendar, even if we changed nothing else, doing this would make era pretty much invisible at runtime for the vast majority of developers who won't ever use that property.

FWIW, the current behavior is what @ptomato said: era: undefined is currently returned for the ISO calendar.

  1. Should era be exposed in TS types for types where it's not used?

Semantically, no. As I said above, I believe this is a TypeScript problem, not a Temporal problem.

The easiest thing to do would be to create TS types that list era and any other built-in calendar properties as optional fields.

  type DateTimeFields = {
    // era: string | undefined; // CURRENT
    era?: string | undefined; // PROPOSED
    yearType?: string | undefined; // PROPOSED
    // other built-in calendars' properties would go here
    year: number;
    month: number;
   //  . . .
}

It might be possible to do something more sophisticated like making calendar-aware types generic, with an ISO default. This would support different fields per-calendar with everything validated by TypeScript. Kinda like this: (totally untested, may not work). Honestly I'm only 50/50 that this would be better than the optional fields approach above.

class TemporalDate<C extends Temporal.CalendarProtocol = Temporal.Calendar.ISO>
  implements ReturnType<C['dateFromFields']> {
  // . . .
  static from(item: Temporal.Date<C> | DateLike<C> | string, options?: AssignmentOptions): Temporal.Date<C>;
  toLocalDateTime(data: { timeZone: Temporal.TimeZone; time?: Temporal.Time }): LocalDateTime<C>;
}

But if it works, it'd enable TS code like this:

dateISO = Temporal.Date.from('2020-01-01');
dateISO.era; // TS compiler error: `era` is not defined on type Temporal.Date<Temporal.ISOCalendar> 

dateJPN = Temporal.Date<Temporal.JapaneseCalendar>.from('2020-01-01[c=japanese]');
// The syntax below might also be possible but not sure:
// dateJPN = Temporal.Date<'japanese'>.from('2020-01-01[c=japanese]');
dateJPN.era; // no error
dateJPN.toLocalDateTime('America/Los_Angeles').era; // also no error. the type is "infectious"

// It's also possible that we could infer the type of the Date via the calendar string literal
// Not 100% sure that would work though.
dateJPN2 = Temporal.Date.from({year: 2020, month: 1, day: 1, calendar: 'japanese').era;

I'm dubious that the generic approach would be worth the extra complexity, but it might be worth a little time to experiment with. It'd definitely be helpful for custom calendars where the optional fields approach wouldn't work.

@ptomato
Copy link
Collaborator

ptomato commented Sep 18, 2020

Meeting, Sept. 18: The change we will make to address this is that era should not be a property on the return value of getFields(), if the calendar doesn't use eras (such as the ISO calendar).

@sffc
Copy link
Collaborator

sffc commented Sep 19, 2020

Conclusions from yesterday's meeting:

  • The ISO-8601 calendar won't return an era from getFields()
  • The question of whether or not the field exists on the prototype is pending Make fields enumerable own (data?) properties on instances #917
  • The "gregory" calendar will have an era field with two possible values, BC and AD, and the year values will always be positive. ISO year 0, 0000-01-01, will be 1 BC in "gregory".

@ptomato
Copy link
Collaborator

ptomato commented Sep 19, 2020

I didn't think the last conclusion (about the "gregory" calendar) was part of what we had decided, to be honest I thought it was facetious, in a (successful) attempt to lighten the mood in the meeting! If we are seriously considering that, then I have some concerns:

  • bce and ce vs. bc and ad vs. capitalized versions of those
  • I already find Intl.DateTimeFormat's silent dropping of the era (in e.g. new Date('-000300-12-01').toLocaleString('en', {year:'numeric'})) to be confusing, it has caused at least one bug in the Temporal polyfill, so I would be concerned about adding even more opportunities for this to happen.

I'm not against adding an era to the gregory calendar but I would like to find a way to do it that avoids these concerns. The rationale "otherwise gregory is the same as iso8601" is not enough IMO. (which is why I thought it was a joke)

@justingrant
Copy link
Collaborator Author

Agree. Historically, the AD vs. CE thing has been a political flashpoint. It'd be nice to avoid it.

@sffc
Copy link
Collaborator

sffc commented Sep 20, 2020

I was serious about the difference between "iso8601" and "gregory" being how they deal with eras.

I also want to emphasize that the "gregory" calendar is not part of 262. It's something that 402 needs to decide on how to standardize. We will need to spin off discussions in 402 about how to handle each and every calendar, "gregory" included.

bce and ce vs. bc and ad vs. capitalized versions of those
Agree. Historically, the AD vs. CE thing has been a political flashpoint. It'd be nice to avoid it.

It's already the case that we will need to make the call about how to name eras in other calendar systems. For identifiers, we should probably stick with lowercase. CLDR uses integers 0 and 1 as the identifiers (I don't know if I agree with that, but it's something to consider).

The English localization of era 0 is "BC" with variant "BCE" and of era 1 is "AD" with variant "CE" (see here). Variants might be triggered based on the display context (see tc39/ecma402#355).

"gregory" is named after the pope and is inherently tied to Christianity, just like the Hebrew, Islamic, and Buddhist calendars to their respective religions. Therefore, religious era names are acceptable. "iso8601" is an international standard that avoids the use of religious era names.

I already find Intl.DateTimeFormat's silent dropping of the era (in e.g. new Date('-000300-12-01').toLocaleString('en', {year:'numeric'})) to be confusing, it has caused at least one bug in the Temporal polyfill, so I would be concerned about adding even more opportunities for this to happen.

You can already explicitly request that the era is outputted.

new Date().toLocaleDateString("en", { year: "numeric", month: "short", day: "numeric", era: "short" })
// "Sep 20, 2020 AD"

If you don't request a specific option, it's up to the datetime pattern matching algorithm to pick the best format for your calendar system. See more discussion in tc39/ecma402#394. That particular case looks like a bug that CLDR should be able to fix by tailoring the era display based on the sign of the year.

@ptomato
Copy link
Collaborator

ptomato commented Sep 25, 2020

OK, then it sounds like this is a question that can be deferred until more calendars are implemented, which I believe was something that Shane's team might work on? In any case if it is in the 402 domain then it's not needed for our Stage 3 review.

@ptomato ptomato removed documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Sep 25, 2020
@ptomato ptomato removed this from the Stable proposal milestone Sep 25, 2020
@ptomato ptomato added this to the Stage 3 milestone Sep 25, 2020
@justingrant
Copy link
Collaborator Author

I don't have an opinion about which milestone this is attached to, but I do think we should have guidance for calendar implementers for how era should be handled in calendars that don't use it.

Also we should make a call about how era is represented in TS types.

Not sure which milestone is important for those two things.

@ptomato
Copy link
Collaborator

ptomato commented Sep 25, 2020

I guess we still do have to implement the other two conclusions from #901 (comment) before freezing the proposal. I'll move this back into the earlier milestone, then.

@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Sep 25, 2020
@ptomato ptomato modified the milestones: Stage 3, Stable proposal Sep 25, 2020
@Louis-Aime
Copy link

If I may add among the guidances to implementers who intent to use the era field in Temporal objects. I would suggest that the era field be a plain integer if it is used in a custom calendar. The representation of this field should be deferred to Intl, since it is the same problem as for the other fields: year, month, day.
IMHO era should always be mapped to a positive integer, the 0 value being the most ancient epoch a calendar handles. But there is not much difference with beginning with 1.

@sffc
Copy link
Collaborator

sffc commented Oct 16, 2020

The main problem with representing eras as numbers is that doing so does not make sense in all calendar systems. In the Japanese calendar, eras are identified by their name in Unicode code points (or potentially a corresponding romanization), and there isn't necessarily a well-defined "first era". Using strings to identify eras is more generalizable.

@ptomato ptomato self-assigned this Oct 19, 2020
ptomato added a commit that referenced this issue Oct 23, 2020
The ISO-8601 calendar should not return an era from getFields(). Only
calendars that require an era, such as the Japanese calendar, should do
so.

See: #901
ptomato added a commit that referenced this issue Oct 23, 2020
As part of this issue we decided that another difference between the ISO
and Gregorian calendars is that the latter has BC/AD eras. It's
indeterminate how this will exactly be standardized, as that's up to
ECMA-402, but we add it to the experimental Gregorian calendar in the
polyfill.

See: #901
Ms2ger pushed a commit that referenced this issue Oct 26, 2020
The ISO-8601 calendar should not return an era from getFields(). Only
calendars that require an era, such as the Japanese calendar, should do
so.

See: #901
Ms2ger pushed a commit that referenced this issue Oct 26, 2020
As part of this issue we decided that another difference between the ISO
and Gregorian calendars is that the latter has BC/AD eras. It's
indeterminate how this will exactly be standardized, as that's up to
ECMA-402, but we add it to the experimental Gregorian calendar in the
polyfill.

See: #901
Ms2ger pushed a commit that referenced this issue Oct 26, 2020
The ISO-8601 calendar should not return an era from getFields(). Only
calendars that require an era, such as the Japanese calendar, should do
so.

See: #901
Ms2ger pushed a commit that referenced this issue Oct 26, 2020
As part of this issue we decided that another difference between the ISO
and Gregorian calendars is that the latter has BC/AD eras. It's
indeterminate how this will exactly be standardized, as that's up to
ECMA-402, but we add it to the experimental Gregorian calendar in the
polyfill.

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

Successfully merging a pull request may close this issue.

4 participants