js-temporal / temporal-polyfill

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

TypeScript: allow Intl.Locale objects in locale list params #224

Open lionel-rowe opened 1 year ago

lionel-rowe commented 1 year ago

This PR allows locales parameters to accept Intl.Locale objects (or arrays thereof). This already works fine in plain JS, but the current TS types are overly restrictive.

See also: https://github.com/microsoft/TypeScript/issues/52946

https://tc39.es/ecma402/#sec-canonicalizelocalelist

CanonicalizeLocaleList is called on all such locales arguments, iterating through them as an array, checking for the [[InitializedLocale]] internal slot (used to identify Intl.Locale objects), and reading the [[Locale]] internal slot if it exists (for Intl.Locale objects, this is the string representation of the locale). Thus 'x'.toLocaleUpperCase('es') behaves identically to 'x'.toLocaleUpperCase(new Locale('es'))

Note that Intl.DateTimeFormat already accepts Intl.Locales — the casting to as ConstructorParameters<typeof IntlDateTimeFormat>[0] is only needed until the TS lib issue is fixed.

12wrigja commented 1 year ago

As far as I can tell, this symbol is relatively new? https://stackoverflow.com/questions/73637484/namespace-intl-has-no-exported-member-localesargument

Presumably, if we add this then our .d.ts file will be incompatible with downstream ts users who don't have a relatively up to date TS. This is typically solved using typesVersions which we don't currently have set up. Maybe we could extend the copy-types.mjs script to also set this up?

justingrant commented 1 year ago

@12wrigja We've solved this for other built-in types by duplicating the types in our own .d.ts. Add long as the types match exactly, this won't break TSC in later versions of TS.

Seems simple to do the same thing here. What do you think?

12wrigja commented 1 year ago

Duplicating is basically what the fallback for typesVersions would do. I'm just suggesting having multiple versions so that we automatically stay up to date as the upstream Intl typings change.

12wrigja commented 1 year ago

And realistically wouldn't the typings that end up in the standard .d.ts for TS itself use the Intl symbols instead of a duplicate?

justingrant commented 1 year ago

I'm just suggesting having multiple versions so that we automatically stay up to date as the upstream Intl typings change.

Given that TS will do declaration merging, what's the advantage of having separate per-TS-version type declaration files that would outweigh the complexity of maintaining separate files?

For example, dayPeriod, dateStyle, and timeStyle are relatively new so we added them into the polyfill's types, but they work fine on later versions of TS because of declaration merging.

https://github.com/js-temporal/temporal-polyfill/blob/c0f7349a327b68543797d38e045b1fd8c1e0949b/index.d.ts#L1526-L1533

And realistically wouldn't the typings that end up in the standard .d.ts for TS itself use the Intl symbols instead of a duplicate?

Yep, but as long as the parts of our types match what's in TS's own lib.* type declarations, then everything should work fine. Unless I'm missing some important point?

lionel-rowe commented 1 year ago

I've duplicated LocalesArgument with a TODO to remove in the future.

12wrigja commented 1 year ago

I'd like to verify this works when compiling against TS versions that don't define LocalesArgument before merging.

12wrigja commented 1 year ago

Ultimately, the PR in it's current state works (as the .d.ts doesn't explicitly reference globalThis.Intl.LocalesArgument), but I did also manage to get this to work in https://github.com/js-temporal/temporal-polyfill/pull/229 using typesVersions and I think it has some advantages:

lionel-rowe commented 1 year ago

Thanks @12wrigja — I've updated PR with that fix. I've set the threshold at <4.7.4, as 4.7.4 has the type but 4.6.4 is missing it.

12wrigja commented 1 year ago

I'm happy with this, but would like @justingrant to take a look too.

12wrigja commented 1 year ago

After reading over https://twitter.com/atcb/status/1634653474041503744?s=46&t=QSggAfTZ89VDmJRcZoeQ0A, I think we might need some small adjustments to the build so that this works consistently (as typesVersions is only used when exports is not).

jnoordsij commented 2 months ago

Just ran into this, as I encountered this typing issue myself when trying to pass some Intl.LocalesArgument typed variable to the toLocaleString function. Is there any chance of this still getting merged at some point?