js-temporal / temporal-polyfill

Polyfill for Temporal (under construction)
ISC License
529 stars 28 forks source link

Temporal.TimeZone.from incorrect return type #171

Closed Darkhogg closed 2 years ago

Darkhogg commented 2 years ago

According to the documentation, Temporal.TimeZone.from should return a TimeZone, but in this polyfill it's defined as returning TimeZone | TimeZoneProtocol.

ptomato commented 2 years ago

Thanks for taking the time to open an issue! It's appreciated.

You can read about this in the Custom Time Zones section — plain objects implementing certain methods (called TimeZoneProtocol in the TypeScript definitions) are allowed to be time zones as well.

A plain object as a time zone is a very rare, specialized use case so that's why we don't mention it directly in the documentation, other than in the Custom Time Zones section. If we mentioned it everywhere, it'd be more likely to confuse people or make them think they might need to use that feature when most likely they don't.

We could probably make the signature of Temporal.TimeZone.from more accurate, e.g. something like

static from<T extends Object>(timeZoneLike: T): T;
static from(timeZoneLike: TimeZoneLike): TimeZone;

(although I'm not exactly sure how to express this in TypeScript)

Feel free to submit a PR to either the TypeScript definitions or the documentation to make this clearer.

Darkhogg commented 2 years ago

Shouldn't .from always return a TimeZone, though? Other methods accepting TimeZoneProtocol or TimeZoneLike seems fine, but in my mind a ' .from' is to convert to the class (and that's what the documentation suggests to me), I might just be wrong.

Actually, I apparently didn't read properly:

This static method creates a new time zone from another value. If the value is another Temporal.TimeZone object, or object implementing the time zone protocol, the same object is returned. If the value is another Temporal object that carries a time zone or an object with a timeZone property, such as Temporal.ZonedDateTime, the object's time zone is returned.

So yes, it seems like .from might just return the thing you pass it, unchanged. So it seems from this description that the overloaded signature you wrote is correct.

ptomato commented 2 years ago

Oh, I guess what I wrote is not quite correct though, because if you pass a ZonedDateTime you'll get its timeZone property.