GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add missing currencyDisplay to resolved number format options by snowystinger · Pull Request #44006 · microsoft/TypeScript

Conversation

@snowystinger

@snowystinger Rob Snow (snowystinger) commented May 8, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@snowystinger
Add missing currencyDisplay to resolved number format options
7366eb2
@typescript-bot

TypeScript Bot (typescript-bot) commented May 8, 2021

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot TypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 8, 2021
@ghost

Deleted user (ghost) commented May 8, 2021 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@snowystinger

Rob Snow (snowystinger) commented May 8, 2021

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@almeidx

Almeida (almeidx) commented May 8, 2021

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
CreativeTechGuy
Jason O'Neill (CreativeTechGuy) reviewed May 9, 2021
View reviewed changes
Comment thread src/lib/es5.d.ts Outdated
style: string;
currency?: string;
currencyDisplay?: string;
currencySign?: string;

@CreativeTechGuy Jason O'Neill (CreativeTechGuy) May 9, 2021

Copy link
Copy Markdown

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@snowystinger Rob Snow (snowystinger) May 10, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@snowystinger Rob Snow (snowystinger) May 10, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) May 19, 2021

Copy link
Copy Markdown
Member

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.

Daniel Rosenwasser (@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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@orta Orta Therox (orta) Aug 30, 2021

Copy link
Copy Markdown
Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@typescript-bot 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
@snowystinger
Move declaration to es2020
bc2ca52
@sandersn
Nathan Shively-Sanders (sandersn) requested review from Daniel Rosenwasser (DanielRosenwasser) and Orta Therox (orta) May 19, 2021 00:07
@sandersn Nathan Shively-Sanders (sandersn) assigned Daniel Rosenwasser (DanielRosenwasser) May 19, 2021
orta
Orta Therox (orta) approved these changes Aug 30, 2021
View reviewed changes

@orta Orta Therox (orta) left a comment

Copy link
Copy Markdown
Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
snowystinger reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@orta Orta Therox (orta) mentioned this pull request Aug 30, 2021
4.5 Intl changes overview #45646
Closed
Orta Therox and others added 2 commits September 8, 2021 10:17
Orta Therox
Merge branch 'main' into add-missing-currencySign-to-resolved-options
fb7a12c
@orta
Update es2020.intl.d.ts
e7d160e
@orta

Orta Therox (orta) commented Sep 8, 2021

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn

Nathan Shively-Sanders (sandersn) commented Sep 13, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@snowystinger

Rob Snow (snowystinger) commented Jan 18, 2022

Copy link
Copy Markdown
Contributor Author

gentle nudge about the tests from main
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Nathan Shively-Sanders (sandersn) added 2 commits January 18, 2022 12:43
@sandersn
Merge branch 'main' into add-missing-currencySign-to-resolved-options
9a20a7c
@sandersn
Fix bad merge
4ede453
@sandersn

Nathan Shively-Sanders (sandersn) commented Jan 18, 2022

Copy link
Copy Markdown
Member

Once the tests pass I'll merge this for 4.6
Thanks Daniel Rosenwasser (@DanielRosenwasser) for remembering this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sandersn
Nathan Shively-Sanders (sandersn) approved these changes Jan 18, 2022
View reviewed changes
@sandersn
Nathan Shively-Sanders (sandersn) merged commit 0822116 into microsoft:main Jan 18, 2022
@snowystinger
Rob Snow (snowystinger) deleted the add-missing-currencySign-to-resolved-options branch January 18, 2022 22:01
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sandersn Nathan Shively-Sanders (sandersn) sandersn approved these changes
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Awaiting requested review from DanielRosenwasser
+2 more reviewers
@CreativeTechGuy Jason O'Neill (CreativeTechGuy) CreativeTechGuy left review comments
@orta Orta Therox (orta) orta approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@DanielRosenwasser Daniel Rosenwasser (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

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@snowystinger @typescript-bot @almeidx @orta @sandersn @CreativeTechGuy @DanielRosenwasser
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.