Reasoning behind code structure

[jens-ox]

Hi all! Here are a couple of questions that came up when initially starting to work on the codebase. As I'm new to the implementation of polyfills, the questions are just for me to understand why things are the way they are and are not meant as criticism or anything like that! All answers are really appreciated 😊

Abstract Code Style

One thing I noticed was that the coding style is quite abstract - e.g. the whole slots.ts file, most mentionable the nine-fold overloaded HasSlot.

Is there a specific reason why such an abstract style was used? For sure there would be some duplications if private fields + get/set would be used instead of the slotting construct, but I'd argue that it would be way more readable. Maybe duplications could even be reduced to a sane amount by using some abstract classes that encapsulate things like validation of common parameters.

Usage of an explicit declaration file

Normally when working on TypeScript codebases I let TypeScript generate the declaration file at build time. Here, there's an explicit declaration file and the classes implement the interfaces given there. Why is this the case? Wouldn't it be less abstract to write types inline? Or is this intended to increase the stability of the interfaces?

Internal object creation methods

@ptomato explained why things like CreateTemporalDate exist, that basically circumvent new PlainDate(...). I don't really understand the requirement that lead to this implementation being necessary. Sure, if the user passes some obscure stuff to the constructor, it will log something - but how does that justify implementing a special object creation method, just to avoid that it logs twice instead of once?

Explicitly declaring methods

A lot of files contain things like const MathMin = Math.min at the start. Is that for performance optimization? Shouldn't calling a static method of a global object already be "optimal"?

"Invalid receiver"

A lot of class methods will explicitly throw a "invalid receiver" type error if called in a context where the DATE_BRAND field is not set. Both the constructor and the corresponding CreateTemporalX method will set this. So is this check necessary?

[12wrigja]

A lot of files contain things like const MathMin = Math.min at the start. Is that for performance optimization? Shouldn't calling a static method of a global object already be "optimal"?

This is to avoid issues where someone clobbers over the global definitions of these at runtime and causes the library to return incorrect results. E.g.

<load temporal polyfill>
Math.min = (a,b) => a; // Redefine math.min to be incorrect
... some temporal operation using Math.min here ...

If we weren't capturing the original definition of Math.min at load time, then we would return incorrect results.

R.e. the standalone .d.ts:

Or is this intended to increase the stability of the interfaces?

I don't think this is stated goal anywhere, but I've previously discussed with @justingrant about how this standalone .d.ts might eventually make it's way into the TypeScript standard library. Eventually, I could also see this being "reversed": we would pull in the typings from the standard .d.ts library and use those instead of our standalone version in order to ensure compatibility.

Another reason for doing this is to avoid circular dependencies between all the types involved.

A lot of class methods will explicitly throw a "invalid receiver" type error if called in a context where the DATE_BRAND field is not set.

This might be to avoid people trying to call these function implementations in contexts that aren't objects created from the Temporal API. E.g. Reflect.apply(Temporal.Instant.prototype.equals, {}, myInstantFromTemporalAPI) should throw, as {} is not an object created by the Temporal APIs.

[ptomato]

All very good questions and it would be great to have the answers documented in this thread. I agree with all of James' answers, and I'll try to answer the ones that haven't been answered yet.

Abstract Code Style

This GetSlot mechanism was inherited when we forked the polyfill from proposal-temporal, and it was originally written that way because the polyfill was started when private class fields weren't yet part of JavaScript. I don't think we could replace it 1-to-1 with private fields, because the semantics of GetSlot are such that the slots are private across all the Temporal types, rather than private to one Temporal type. There's no way to express that directly with private class fields.

Nonetheless, I did already have a vague idea of how it could be refactored. I thought there was an issue or discussion where I outlined this already, but I can't find it. So stop me if you've heard this before. If I were starting from scratch, I'd use a PIMPL idiom to expose whatever we need for the internals of Temporal, while exposing nothing on the public API surface, and making sure that nothing performed on the Impl object was observable. This would make the giant overwhelming ecmascript.ts file a lot smaller and better organized, and eliminate a lot of the gymnastics we have to resort to for avoiding observable method calls. It would look roughly like this code example: (not tested, just written off the cuff so probably full of mistakes)

Code example (click to expand)

class PlainDate {
  readonly #impl: PlainDateImpl;
  readonly #brand;
  constructor(isoYear: number, isoMonth: number, isoDay: number, calendar?: ...) {
    // (validate arguments so that PlainDateImpl is only called with valid ones)
    this.#impl = new PlainDateImpl(isoYear, isoMonth, isoDay, calendar);
  }

  // example method, delegates to PlainDateImpl
  equals(other) {
    if (!(#brand in this)) throw new TypeError('invalid receiver');
    otherImpl = PlainDateImpl.cast(other);  // performs ToTemporalDate and returns the PlainDateImpl
    return this.#impl.equals(otherImpl);
  }
}

class PlainDateImpl {
  readonly #isoYear: number;
  readonly #isoMonth: number;
  readonly #isoDay: number;
  readonly #calendar: CalendarProtocol;
  constructor(isoYear: number, isoMonth: number, isoDay: number, calendar: CalendarProtocol) {
    this.#isoYear = isoYear;
    this.#isoMonth = isoMonth;
    this.#isoDay = isoDay;
    this.#calendar = calendar;
  }

  // example method implementation
  equals(other: PlainDateImpl): bool {
    return this.#isoYear == other.#isoYear &&
      this.#isoMonth == other.#isoMonth &&
      this.#isoDay == other.#isoDay &&
      CalendarImpl.get(this.#calendar).equals(CalendarImpl.get(other.#calendar));
  }
}

I'm not actually sure if it's worth doing this refactor at this point. Maybe it would be a lot of churn for nothing, or maybe it would be worth the increase in maintainability for the rest of this polyfill's lifetime.

There are probably a bunch of improvements we could make in the shorter term. I think the only reason we need 9 overloads of HasSlot is because HasSlot accepted more than one slot name as arguments in the original proposal-temporal polyfill. This was convenient in untyped JavaScript, but it seems like the problem is that this is really difficult to express to the TypeScript compiler, so maybe it's just not worth having that convenience and we should make HasSlot just take one slot name at a time.

Internal object-creation methods

If Temporal were a regular library and not a polyfill we probably wouldn't go to the trouble of having these extra methods (to be honest, nor the hassle of having brand checks for invalid receiver, nor saving pristine copies of methods like Math.min, for that matter). As it is though, a polyfill must adhere to the specification, and the specification says that only one observable property access must occur in the case of the code snippet that I wrote in that other thread.

(Note that if we were to use the PIMPL pattern I suggested above, it seems likely to me that these internal methods would go away — we'd just create a PlainDateImpl instead of calling CreateTemporalDate.)