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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lionel-rowe
Copy link

@lionel-rowe lionel-rowe commented Feb 24, 2023

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: microsoft/TypeScript#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.

@lionel-rowe lionel-rowe marked this pull request as draft February 24, 2023 05:11
@lionel-rowe lionel-rowe marked this pull request as ready for review February 24, 2023 05:30
lib/intl.ts Show resolved Hide resolved
@12wrigja 12wrigja self-requested a review February 27, 2023 18:32
@12wrigja
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

temporal-polyfill/index.d.ts

Lines 1526 to 1533 in c0f7349

export interface DateTimeFormatOptions extends Omit<globalThis.Intl.DateTimeFormatOptions, 'timeZone' | 'calendar'> {
calendar?: string | Temporal.CalendarProtocol;
timeZone?: string | Temporal.TimeZoneProtocol;
// TODO: remove the props below after TS lib declarations are updated
dayPeriod?: 'narrow' | 'short' | 'long';
dateStyle?: 'full' | 'long' | 'medium' | 'short';
timeStyle?: 'full' | 'long' | 'medium' | 'short';
}

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
Copy link
Author

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

Copy link
Contributor

@justingrant justingrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. LGTM! @12wrigja feel free to merge if you think this change is OK.

@12wrigja
Copy link
Contributor

12wrigja commented Mar 1, 2023

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

@12wrigja
Copy link
Contributor

12wrigja commented Mar 2, 2023

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 #229 using typesVersions and I think it has some advantages:

  • It leaves the index.d.ts file clean and enables automatic forward compatibility with any changes to the upstream definition of Intl.LocalesArgument (the real definition of which is decided by our users, not whatever version of TS we compile against).
  • It moves all the older version cruft into a separate .d.ts file, making cleanup easy (just delete the file and the corresponding lines in package.json and done). In this case, what we are effectively doing is providing the typedef for Intl.LocalesArgument to older versions of TS if it's not available in the standard typings.
  • Given that typings don't tend to make backwards-incompatible changes and we really only need additions to the .d.ts, we can continue to use this pattern for other types that we've coped in so long as we can figure out what TS version it should start applying at.

Thoughts @justingrant?

@lionel-rowe I'd be more than happy for you to copy my WIP PR above and fix it up if you'd still like to contribute this fix.

@lionel-rowe
Copy link
Author

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
Copy link
Contributor

12wrigja commented Mar 4, 2023

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

@12wrigja
Copy link
Contributor

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).

Based on microsoft/TypeScript#50890 (comment).

Copy link
Contributor

@justingrant justingrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Should we also consider a similar trick for our existing re-declaration below?

    // TODO: remove the props below after TS lib declarations are updated
    dayPeriod?: 'narrow' | 'short' | 'long';
    dateStyle?: 'full' | 'long' | 'medium' | 'short';
    timeStyle?: 'full' | 'long' | 'medium' | 'short';

@jnoordsij
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants