Skip to content

Add support for NumberFormatOptions notation #37721

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

Merged
merged 4 commits into from
Jun 16, 2020

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Mar 31, 2020

Closes #36533

Adds the notation option to NumberFormatOptions in src/lib/es5.d.ts

I'm an experienced JS developer, but new to TypeScript, so if any changes are required to this PR you may have to give me some pointers.

@msftclas
Copy link

msftclas commented Mar 31, 2020

CLA assistant check
All CLA requirements met.

@NarasimhaReddyY
Copy link

This should be merged.

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Apr 6, 2020
@sandersn sandersn self-assigned this Apr 6, 2020
@benmccann
Copy link
Contributor Author

Thanks for assigning this to yourself @sandersn. Any chance you'd be able to take a quick look? I'm hoping it should be an easy review since it's only a couple lines.

@sandersn
Copy link
Member

I typically merge lib changes at the beginning of a version to let people find breaks, so I'll wait until 4.0 in a few weeks. Touching the ES5 lib is bound to break somebody, no matter how small the change.

@maneetgoyal
Copy link

maneetgoyal commented Apr 17, 2020

@benmccann @awibox Should this property go in es5.d.ts or esnext.intl.d.ts instead?

I was just trying to draw some analogy with the Array object. All the common Array methods like forEach, map, etc. are in es5.d.ts. But the new ones like .flatMap and .flat are in es2019.array.d.ts.

notation recently became a standard property (right?). ECMAScript 2021 Intl specs (draft) makes a mention of it. No mention in ECMAScript 2019 Intl specs. So should it go in esnext.intl.d.ts/es2021.intl.d.ts instead of es5.d.ts?


Edit: Just realizing, may be the location doesn't have anything to do ECMAScript and rather just depends upon browser support?

@BPScott
Copy link

BPScott commented Jun 14, 2020

Yo @sandersn, I'm guessing we've missed the boat for merging this and #38013 and we'll have to wait till the 4.1 cycle begins?

@sandersn
Copy link
Member

Sorry for missing this. This should go into ES2020, not ES5, but if we can get it in before the beta on Monday it'll be fine to ship. I'm pretty sure the right place is es2020.intl.d.ts.

@benmccann I'll do it in the next day or so if I have time. I understand if you're sick of me forgetting about this. =)

@benmccann
Copy link
Contributor Author

Great. Thanks so much @sandersn ! I'll leave the rest to you then. I'm pretty new to TypeScript, so I'm sure you have a much better handle on things than I do, but please let me know if there's anything I can do to help

@sandersn sandersn merged commit e97003f into microsoft:master Jun 16, 2020
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
Archived in project
Development

Successfully merging this pull request may close these issues.

NumberFormatOptions is missing several options including notation
7 participants