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

Add missing parameters from Array.toLocaleString on ES2015 libs #57679

Merged
merged 15 commits into from
Mar 27, 2024

Conversation

neilbryson
Copy link
Contributor

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Mar 7, 2024
Copy link
Member

@sandersn sandersn left a 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.

*
* [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;
Copy link
Member

Choose a reason for hiding this comment

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

  1. Does NumberFormatOptions & DateTimeFormatOptions cover everything? I see that BigInt also has toLocaleString. Could other types have it too?
  2. since this adds an overload, locales should not be optional. Otherwise a zero-argument call might resolve to this when it should resolve to the es5 overload.
Suggested change
toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string;
toLocaleString(locales: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string;

Copy link
Contributor Author

@neilbryson neilbryson Mar 21, 2024

Choose a reason for hiding this comment

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

Thanks for the comments!

  1. 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 toLocaleString signature of TypedArrays. I'll send a commit for that

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

Copy link
Member

Choose a reason for hiding this comment

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

  1. 👍🏼
  2. The error makes it look like both locales and options are both required. But only locales should be required; options should 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

Copy link
Contributor Author

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

*
* [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;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string;
toLocaleString(locales: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string;

@sandersn sandersn self-assigned this Mar 20, 2024
Copy link
Member

@sandersn sandersn left a 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.

*
* [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;
Copy link
Member

Choose a reason for hiding this comment

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

  1. 👍🏼
  2. The error makes it look like both locales and options are both required. But only locales should be required; options should 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

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good!

@sandersn sandersn merged commit 32a1370 into microsoft:main Mar 27, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing arguments in "toLocaleString" method of Array
3 participants