Skip to content

Add missing currencyDisplay to resolved number format options #44006

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jan 18, 2022
Merged

Add missing currencyDisplay to resolved number format options #44006

merged 6 commits into from
Jan 18, 2022

Conversation

snowystinger
Copy link
Contributor

@snowystinger snowystinger commented May 8, 2021 •
edited
Loading

Please verify that:

  • There is an associated issue in the Backlog milestone (required)
  • Code is up-to-date with the master branch
  • You've successfully run gulp runtests locally
  • There are new or updated unit tests validating the change

Refer to CONTRIBUTING.MD for more details.
https://github.com/Microsoft/TypeScript/blob/master/CONTRIBUTING.md

Fixes #43958

@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 8, 2021
@ghost
Copy link

ghost commented May 8, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@snowystinger
Copy link
Contributor Author

I'm not sure why it thinks I don't have a linked issue, it's in the description. Probably didn't format that correctly.

@almeidx
Copy link

almeidx commented May 8, 2021

I'm not sure why it thinks I don't have a linked issue, it's in the description. Probably didn't format that correctly.

I believe it expects you to just include the #number (Fixes #43958) of the pull request, but you included the whole URL.

CreativeTechGuy
CreativeTechGuy reviewed May 9, 2021
View reviewed changes
src/lib/es5.d.ts Outdated
@@ -4311,6 +4311,7 @@ declare namespace Intl {
style: string;
currency?: string;
currencyDisplay?: string;
currencySign?: string;
Copy link

Choose a reason for hiding this comment

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

Shouldn't this, and currencyDisplay, go in the es2020.intl.d.ts interface instead?

From MDN Compatibility Chart it looks like this was released at the same time as all of the other options listed in the es2020 interface.

Copy link
Contributor Author

@snowystinger snowystinger May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

my bad, I saw it was added to the available options in es5 here https://github.com/microsoft/TypeScript/pull/40709/files
so I assumed it would also be in the resolved options at that point
I'm happy to move it

Copy link
Contributor Author

@snowystinger snowystinger May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

moved, though I'm not 100% on how I set up the tests for it
also, worried that while it fixes a bug, it's a bit breaking because it was added into es5 half a year ago

Copy link
Member

Choose a reason for hiding this comment

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

@DanielRosenwasser previously commented that Intl isn't part of the ecmascript standard, so I think it should actually stay in ES5. Daniel, can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

We want this PR as it is, es2020 is the right pattern - these APIs may not tie directly to the esXXXX specs but they are released and supported in a similar way to how we structure these libs. This came out in 2020 and we should reflect that via the target - which this PR does.

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 10, 2021
orta
orta approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

This PR is good and we should get it in for 4.5 - I'll give Daniel a few days to give any last opinions and I'll merge otherwise.

@orta orta mentioned this pull request Aug 30, 2021
Closed
@orta
Copy link
Contributor

orta commented Sep 8, 2021

I've rebased this after #45647 got merged and assuming it's green, I'll merge tomorrow

@sandersn
Copy link
Member

Looks like the new tests are still failing after the merge from main.

@snowystinger
Copy link
Contributor Author

gentle nudge about the tests from main

@sandersn
Copy link
Member

Once the tests pass I'll merge this for 4.6
Thanks @DanielRosenwasser for remembering this.

@sandersn sandersn merged commit 0822116 into microsoft:main Jan 18, 2022
@snowystinger snowystinger deleted the add-missing-currencySign-to-resolved-options branch January 18, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CreativeTechGuy CreativeTechGuy CreativeTechGuy left review comments

@orta orta orta approved these changes

@sandersn sandersn sandersn approved these changes

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

Assignees

@DanielRosenwasser DanielRosenwasser

Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ResolvedNumberFormatOptions is missing currencySign option
7 participants
@snowystinger @typescript-bot @almeidx @orta @sandersn @CreativeTechGuy @DanielRosenwasser