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

Decide TypeScript type of ZonedDateTime.prototype.timeZone #1041

Closed
ptomato opened this issue Oct 22, 2020 · 4 comments
Closed

Decide TypeScript type of ZonedDateTime.prototype.timeZone #1041

ptomato opened this issue Oct 22, 2020 · 4 comments
Labels
later-production-polyfill typescript Related to the TypeScript bindings

Comments

@ptomato
Copy link
Collaborator

ptomato commented Oct 22, 2020

Follow-up from #1037/#810.

The current type of the ZonedDateTime.prototype.timeZone property is TimeZoneProtocol in the TypeScript bindings. This is annoying because the methods in the protocol are optional, and so you have to write an extra as Temporal.TimeZone to deal with it in the 99.9% case, whereas the case where you have purposely introduced a TimeZoneProtocol object without all the methods is very rare. But on the other hand, leaving it as-is leaves maximal choice for the code author.

@ryzokuken
Copy link
Member

Wait, don't all Temporal.TimeZone objects implement the TimeZoneProtocol interface?

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 23, 2020

They do! But not all objects implementing the TimeZoneProtocol interface have all the methods of Temporal.TimeZone.

@justingrant
Copy link
Collaborator

justingrant commented Oct 23, 2020

Wait, don't all Temporal.TimeZone objects implement the TimeZoneProtocol interface?

They do. The problem is that most of the properties and members are optional, which means that anyone using .id, .getOffsetString(), or any of the other optional methods will get a compiler error. The error can be overridden with a type cast e.g. foo as TimeZone, but requiring an extra 15-character type cast on methods that will essentially always be implemented is confusing the first time and annoying subsequently.

Given the docs changes that Philip added in #1043, I think we're safe in changing the type of .timeZone to Temporal.TimeZone, because our docs will clearly state that:

However, most other code will assume that custom time zones act like built-in Temporal.TimeZone objects.
If you want to interoperate with libraries or other code that you didn't write, then you should implement all the other Temporal.TimeZone methods and properties

This seems like a strong enough guarantee that code that follows best practices will quack like a TimeZone. If developers aren't following best practices, then they can always override our TS types to require more strictness. A good way to think about TS types is like lint rules-- because they're customizable by the developer they don't have to be perfect, and corner cases of users not following the docs or doing weird things shouldn't break the DX of everyone else.

@ptomato
Copy link
Collaborator Author

ptomato commented Jan 2, 2023

This issue no longer applies to the proposal. Polyfills will have to decide this for themselves.

@ptomato ptomato closed this as completed Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
later-production-polyfill typescript Related to the TypeScript bindings
Projects
None yet
Development

No branches or pull requests

3 participants