Should built-in calendars be subclasses?

[Ms2ger]

» Temporal.Calendar.from("iso8601").constructor
function ISO8601() { ... }

We should spec those or make them invisible to JS.

[ptomato]

What would be the way to hide the implementation from JS? I think that is better (though I don't really have any justification for that, and I would be fine to go the other way too.)

[ljharb]

Absolutely they should be subclasses of a common base class - Temporal needs to establish a convention for userland calendar objects to follow.

[ptomato]

Thinking about this some more — there is an inconsistency in the way we do time zones and calendars which has bothered me for a while.

In pseudocode, Calendars:

class Calendar {
  constructor(id) {
    this.[[Identifier]] = id;
  }
  method() {
    throw new Error('not implemented');
  }
}
class ISO8601 extends Calendar {
  constructor() { super('iso8601'); }
  method() {
    /* ISO calendar implementation of method */
  }
}
class Japanese extends Calendar {
  constructor() { super('japanese'); }
  method() {
    /* Japanese calendar implementation of method */
  }
}

But TimeZones:

class TimeZone {
  constructor(id) {
    if (new.target === TimeZone)
      throwIfIdentifierIsNotBuiltin(id);
    this.[[Identifier]] = id;
  }
  method() {
    if (isOffsetTimeZoneIdentifier(this.[[Identifier]]))
      /* offset time zone implementation of method */;
    else
      /* named time zone implementation of method */;
  }
}

So, Calendar does expose subclasses to JS, and TimeZone does not. (I guess I answered my own question from #847 (comment) with that.)

It seems to me that we should go with either one or the other (either hide the Calendar subclasses from JS, or expose OffsetTimeZone and NamedTimeZone to JS.)

I think I prefer the latter, as Jordan pointed out it establishes the proper convention. I'm not too concerned about adding more types to Temporal in this way, as I otherwise would be, since these subclasses will mostly be invisible and normal user code doesn't have to care about them.

[ljharb]

Adding them as, eg, TimeZone.Offset, may also keep them neatly tucked away.

[Ms2ger]

How many calendars are we specifying, in that case?

[ptomato]

I think we are only specifying ISO8601, at least in ECMA-262; see also #541

[littledan]

We previously concluded that there would be just one built-in calendar class in #300 . Did anything change to motivate this additional complexity?

[ptomato]

That is right, we did, and I forgot about that.

One reason for the additional complexity is that some calendars will inherit from the ISO calendar (e.g. gregory, japanese) and some will not (e.g. hebrew, islamic). Since these are all built-in calendars, that doesn't matter (they could inherit internally but not have the classes be exposed) but given the split in built-in calendars I'd expect a similar split to occur in userland calendars.

This is probably not something that we have to resolve in order to call all design decisions complete, as it's more of an implementation detail, but it needs to be resolved before we can call the proposal stable.

[justingrant]

I have no opinion about the subclass-vs-not topic in this issue, but a very strong opinion about how built-in calendars should be exposed into the Temporal public API namespace.

If we're putting built-in calendars into the Temporal namespace, then seems much better to make them static properties on the Calendar class, e.g. Temporal.Calendar.ISO8601, Temporal.Calendar.Islamic vs. having all N built-in calendars exposed directly off Temporal, e.g. Temporal.IslamicCalendar. If we put calendars on the Calendar object, then we won't pollute IDE and browser-devtools auto-complete with a big list of calendars (most of which most developers will never use) which would make it harder for users to find the Temporal types that they use frequently.

I think this is similar feedback that @ljharb gave in #847 (comment). I disagree with Jordan about time zones because there's no Offset subclass of TimeZone and also there's no statically-exposed list of built-in time zones. It's all data-driven and will change at runtime. But I strongly agree re: calendars because they're subclasses and also because (unlike time zones) there's a static and small list of built-in calendars.

Regardless of how this is decided, remember to update the TS types so the new API will be discoverable!

[littledan]

I don't see why we'd include any of these as properties; I thought Temporal.Calendar was enough.

[ptomato]

@littledan Would you find the reason I gave above in #847 (comment) (many userland calendars would likely inherit from a specific built-in calendar) sufficient for including them as properties?

[littledan]

I thought the idea was that all of the built-in calendars, including japanese, hebrew, islamic, buddhist, etc would be instances of Temporal.Calendar, not subclasses.

[ptomato]

That was originally the idea, but that seems inconvenient for userland calendars who want to inherit most of the behaviour from one built-in calendar. (For example, it's my understanding from interviews that it's common to have a Hijri calendar that allows a correction of one or two days in either direction for when the moon sighting doesn't agree with the tabular data.)

[littledan]

@ptomato Couldn't those just call super('hijiri') in their constructor to make sure that their internal slots are filled with that behavior?

[ptomato]

That would give you the wrong value in your internal [[Identifier]] slot and then you'd have to override toString() in your subclass. (I don't know if that's a big objection or not)

[littledan]

@ptomato How would this be different if we created different subclasses?

[Ms2ger]

Presumably you'd do something like

class MyHijiriCalendar extends Temporal.HijiriCalendar {
  constructor() {
    super('my-hijiri')
  }
}

Once you're subclassing, implementing toString doesn't seem like a huge burden, though. (We'd need to double-check that the spec doesn't use [[Identifier]] anywhere besides the superclass toString; I don't think it does.)

[ptomato]

I think maybe the question will be clearer if I provide full code examples of doing it one way and doing it the other way, that we can compare side-by-side; I'll do that within the next few days.

[ptomato]

OK, as promised, the example use case of implementing a Hijri calendar with date correction:

// Subclassing way:

const HijriCalendar = Temporal.Calendar.from('islamic').constructor;
// or there is some object called e.g. Temporal.Calendar['islamic']

class AdjustedHijriCalendar extends HijriCalendar {
  #deltaDays;
  constructor(deltaDays) {
    this.#deltaDays = deltaDays;
    super('adjusted-hijri');
  }

  #adjustDate(date) {
    return date.subtract(this.#deltaDays);
  }

  day(date) {
    return super.day(this.#adjustDate(date));
  }
  // etc.
}

Here, super.day() is Temporal.Calendar.from('islamic').constructor.prototype.day() which returns the Hijri calendar day from the internal [[ISOYear]], [[ISOMonth]], and [[ISODay]] slots.

// Switching on identifier way:

class AdjustedHijriCalendar2 extends Temporal.Calendar {
  #deltaDays;
  constructor(deltaDays) {
    this.#deltaDays = deltaDays;
    super('islamic');
  }

  #adjustDate(date) {
    return date.subtract(this.#deltaDays);
  }

  day(date) {
    return super.day(this.#adjustDate(date));
  }
  // etc.

  toString() {
    return 'adjusted-hijri';
  }
}

Here. super.day() is Temporal.Calendar.prototype.day(), which internally looks something like this in pseudocode:

day(date) {
  const id = this.[[Identifier]];
  switch (id) {
    case 'iso8601':
    case 'gregory':
      return date.[[ISODay]];
    case 'islamic':
      return calculateIslamicDay(date.[[ISOYear]], date.[[ISOMonth]], date.[[ISODay]]);
    // etc.
  }
}

Since the [[Identifier]] slot is used for determining the internal behaviour, the super() call in the constructor effectively takes over the role of the extends clause in the class definition. I find this weird and not in line with the way subclassing works elsewhere in JS that many developers would be familiar with.

Note that you also have to override toString(), otherwise you get new AdjustedHijriCalendar2().toString() === 'islamic', but that seems relatively harmless, though surprising.

[justingrant]

What implications (if any) does this change have on time zone subclassing? Does a custom time zone subclass require calling super with a valid ID for a built-in time zone? Or is any ID ok? I'm asking specifically in regards to my NYSE Time Zone cookbook sample PR #1177.

[ptomato]

Oh, glad you reminded me that it should have implications for time zones as well, I forgot all about that. I'll update the PR.

For the NYSE time zone, the only change you would need to make is to add constructor() { super('America/New_York'); }.

[justingrant]

For the NYSE time zone, the only change you would need to make is to add constructor() { super('America/New_York'); }.

But the behavior of that time zone is not consistent with America/New_York when the market is closed. Is that ok? What about a time zone that doesn't behave like any built-in time zone?

[ljharb]

It seems like there should be a base class that accepts any arbitrary ID, or none at all, otherwise you couldn’t make a reasonable subclass that was anything beyond a slight deviation from a builtin calendar.

[justingrant]

If a subclass's behavior deviates from the base class, then should it use the parent's ID or some other arbitrary ID when initializing super?

For a specific example, in the NYSE time zone in #1177, @ptomato you recommended that I shouldn't implement getOffsetStringFor and instead rely on the base class implementation call my custom getOffsetNanosecondsFor and turn its result into a string? Or will the base class emit the string using America/New_York instead of my custom offset value?

The general version of this question is which methods can a subclass safely not implement if it passes a built-in ID to super()? For time zones, my specific question is with the id property and the getOffsetStringFor method-- can I rely on those always delegating to my custom implementation, not to an implementation using the internal ID slot? Not sure about what's the equivalent question for calendars.

A second issue: is the built-in calendar is observable to users of a subclass? If it is, then that seems like a problem because a caller might have code like this:

if (date.calendar.id==='hijri') doSomething();

And assuming that your subclass deviates from the built-in calendar's behavior (because if it didn't deviate, why bother subclassing?) then that code above might be wrong because the calendar isn't really acting like the built-in one in all cases.

Certainly in the NYSE custom calendar case, you'd not want the subclass to expose its underlying time zone ID because most of the time (because the market is open 22% of the hours in a week) the results won't match the builtin America/New_York zone.