calendar on TimeLike TS type causes TS error in PlainTime.from for ZDT/PDT arguments

[justingrant]

The TimeLike TS type is the type of "PlainTime-like" plain objects that can be used in place of a PlainTime instances in Temporal methods like ZonedDateTime.p.withPlainTime or PlainTime.from.

Currently this type looks like this:

  export type TimeLike = {
    hour?: number;
    minute?: number;
    second?: number;
    millisecond?: number;
    microsecond?: number;
    nanosecond?: number;
    calendar?: Temporal.Calendar | 'iso8601';
  };

The problem is that the calendar property is the Calendar type instead of Temporal.CalendarProtocol. The latter is what's used for this property in all the other Temporal types' XxxLike TS types.

I understand why it was typed like this because (unlike other Temporal types) you can't give a PlainTime a userland calendar, so it will always be a Calendar, not a CalendarProtocol. So when initializing a PlainTime with a TimeLike object, you want to restrict the input to Temporal.Calendar | 'iso8601' because non-ISO calendars and userland calendars aren't allowed.

The problem is that calendar of PlainDateTime or ZonedDateTime is a CalendarProtocol. This means that TS thinks that it's illegal to pass a PDT or ZDT instance into PlainTime.from, because PDT and ZDT don't satisfy the TimeLike type, because you can't implicitly cast a CalendarProtocol to a Calendar. Example:

Temporal.PlainTime.from(Temporal.Now.zonedDateTimeISO());
// TS: Argument of type 'ZonedDateTime' is not assignable to parameter of type 'string | PlainTime | TimeLike'.

One solution may be to replace TimeLike with TimeLike | Temporal.ZonedDateTime | Temporal.PlainDateTime everywhere TimeLike is used.

Or is there a better solution? @12wrigja, what do you think?

Also, #1588 is discussing whether PlainTime.p.calendar should be removed. If it is, this problem goes away.

[justingrant]

One solution may be to replace TimeLike with TimeLike | Temporal.ZonedDateTime | Temporal.PlainDateTime everywhere TimeLike is used.

A more DRY solution is to add | Temporal.ZonedDateTime | Temporal.PlainDateTime to the declaration of TimeLike.

[12wrigja]

Forgive my lack of knowledge on the Temporal types, but is it valid to pass a ZonedDateTime or a PlainDateTime everywhere? A scan of the .d.ts reveals the type is used in the following places:

PlainDate#toPlainDateTime
PlainDate#toZonedDateTime

PlainDateTime#withPlainTime

PlainTime#from
PlainTime#compare
PlainTime#equals
PlainTime#with
PlainTime#until
PlainTime#since

ZonedDateTime#withPlainDateTime

TimeLike seems somewhat similar to the other XLike types in Temporal (YearMonthLike, MonthDayLike, DateTimeLike, DateLike, DurationLike), and I think it makes sense to keep it that way, and maybe create a new time that unions these types together and use that everywhere (to get the benefits of DRY but avoid conflating the simplicity of the existing TimeLike type).

Another observation: TimeLike is very commonly used alongside Temporal.PlainTime and string like so: Temporal.PlainTime | TimeLike | string - maybe that type should be given a common name?

[ptomato]

That's a good observation, it might be good to call TimeLike the union of "anything that can be passed where a PlainTime is expected" which is actually Temporal.PlainTime | Temporal.PlainDateTime | Temporal.ZonedDateTime | string | (the type formerly known as TimeLike). This corresponds better to the intention of the "fast-paths" discussed in #1428. Same for the other XLike types.

[12wrigja]

That also sounds reasonable to me too - the fewer types involved the easier Temporal will be to adopt by TS users.

It would be good to try and get this right before it reaches widespread adoption given that the types are likely to end up in the typescript repo and can be hard to get changed later.

[justingrant]

Forgive my lack of knowledge on the Temporal types, but is it valid to pass a ZonedDateTime or a PlainDateTime everywhere?

Yes, as long as "everywhere" means all the places where a TimeLike is currently accepted.

It would be good to try and get this right before it reaches widespread adoption given that the types are likely to end up in the typescript repo and can be hard to get changed later.

Yep, definitely agree! For the same reason, we should probably rename these types to align with the final names of the Temporal classes, e.g. TimeLike => PlainTimeLike

Another observation: TimeLike is very commonly used alongside Temporal.PlainTime and string like so: Temporal.PlainTime | TimeLike | string - maybe that type should be given a common name?

If my fuzzy memory is correct, I believe that the original genesis of the XxxLike types was for DRY typing of both parameters to Temporal methods and for the return value of the (now removed) fields method. The latter was cast to remove the optional-ness of the properties, but the properties were the same so it made sense to have a shared type for both. Now that there's no more fields method, we may have more flexibility.

That said, I vaguely remember a problem with having the XxxLike types include string. I think (although not 100% sure) it was this:

function f(d: Temporal.DateLike = { year: 2020, month: 1, day: 1 }) {
  return Temporal.PlainTime.from(d).toString();
  // expected: TS error
  // actual: no error, because `string` is valid for all `from` methods
}

So I'm not sure that including string in these types would be good, because it'd get more people to use a TS type that might get them into trouble in unexpected ways. I think (again not 100% sure) that the reason we accepted the non-DRY approach of manually including string | XxxLike in parameter type declarations was to avoid an umbrella type for "anything you can pass to Xxx.from" that callers would be tempted to use dangerously. Again, this was a long time ago so if there's a better argument to justify creating some umbrella types, then I'd be open to it.

maybe create a new time that unions these types together and use that everywhere (to get the benefits of DRY but avoid conflating the simplicity of the existing TimeLike type).

@12wrigja, could you share what you had in mind?

[12wrigja]

we should probably rename these types to align with the final names of the Temporal classes, e.g. TimeLike => PlainTimeLike

Definitely, though I'd think this might be a breaking change if there are users using the types from the d.ts in their own code, so probably best to do this only once now rather than later when we consider this stable.

I think I prefer Philip's suggestion that we try and combine together as many types as make sense and given them the name that is final / appears within the Temporal spec/docs/etc. If we then need another type to split out strings from this, I think we should be able to do something like Omit<TemporalName, string> to get that and keep it in sync with the other types (and we could name that too if it was used widely within Temporal source). When I have more time to work on Temporal I'll read over the public API and see what names make sense, but if someone else has time and wants to pick this up that sounds good too.

[ptomato]

It's not breaking in the TC39 sense, since TypeScript is its own thing not standardized by TC39. (All polyfills that I'm aware of are still at 0.x, so it seems OK to have a breaking change there.)

[justingrant]

Yep, was more thinking of "breaking" as "annoying for early adopters using TS", so esp. if we're going to change names of exported types, we should do it soon.

When I have more time to work on Temporal I'll read over the public API and see what names make sense, but if someone else has time and wants to pick this up that sounds good too.

OK, in the short term I'll build a PR that:

1. Adds PlainDateTime and ZonedDateTime to the TimeLike declaration
2. Renames the XxxLike types to use the actual Temporal type names, e.g. TimeLike => PlainTimeLike

Then if someone (James or anyone else, including the TypeScript team) wants to do more major surgery later, that can be a separate PR. Holler if you disagree, otherwise I'll send a PR soon for both repos.