-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Add missing parameters from Array.toLocaleString on ES2015 libs #57679
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
Add missing parameters from Array.toLocaleString on ES2015 libs #57679
Conversation
sandersn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions, plus I need to find out what we need to do legally to use MDN text in Typescript.
src/lib/es2015.core.d.ts
Outdated
| * | ||
| * [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString) | ||
| */ | ||
| toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Does NumberFormatOptions & DateTimeFormatOptions cover everything? I see that BigInt also has toLocaleString. Could other types have it too?
- since this adds an overload,
localesshould not be optional. Otherwise a zero-argument call might resolve to this when it should resolve to the es5 overload.
| toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; | |
| toLocaleString(locales: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments!
-
When I checked, it looks like there are only NumberFormatOptions and DateTimeFormatOptions which are accepted by JS in all types with this method (Object, Array, Number, BigInt, TypedArray). BigInt arrays are covered by the current definitions upon testing, but I will still need to update the
toLocaleStringsignature of TypedArrays. I'll send a commit for that -
I tried not making it optional, but it seems to stop working with single argument calls based on my test run
arrayToLocaleStringES2015.ts(4,11): error TS2575: No overload expects 1 arguments, but overloads do exist that expect either 0 or 2 arguments.
arrayToLocaleStringES2015.ts(10,14): error TS2575: No overload expects 1 arguments, but overloads do exist that expect either 0 or 2 arguments.
I think they should be kept optional, what do you think? In my baselines errors, it was showing the expected behaviour for es5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 👍🏼
- The error makes it look like both
localesandoptionsare both required. But onlylocalesshould be required;optionsshould stay optional. When I did a quick local test with a test project using es2015, all 3 calls below worked:
const a = [1,2,3]
a.toLocaleString()
a.toLocaleString('en-US')
a.toLocaleString('en-US', {style: 'currency', currency: 'USD'}) // thanks copilotThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! I applied the changes to es2015
src/lib/es2015.core.d.ts
Outdated
| * | ||
| * [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString) | ||
| */ | ||
| toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; | |
| toLocaleString(locales: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
sandersn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like locales to be required and options to be optional.
src/lib/es2015.core.d.ts
Outdated
| * | ||
| * [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString) | ||
| */ | ||
| toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 👍🏼
- The error makes it look like both
localesandoptionsare both required. But onlylocalesshould be required;optionsshould stay optional. When I did a quick local test with a test project using es2015, all 3 calls below worked:
const a = [1,2,3]
a.toLocaleString()
a.toLocaleString('en-US')
a.toLocaleString('en-US', {style: 'currency', currency: 'USD'}) // thanks copilot
sandersn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Fixes #57603
Added on both
ArrayandReadonlyArrayin es2015.core.d.tshttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString