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

feat(46907): Add Intl.ListFormat type declarations #47254

Merged
merged 6 commits into from
Apr 5, 2022

Conversation

predetermined
Copy link
Contributor

Fixes #46907

@ghost
Copy link

ghost commented Dec 26, 2021

CLA assistant check
All CLA requirements met.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Dec 26, 2021
@predetermined predetermined changed the title feat(46907): Add ListFormat type declarations feat(46907): Add Intl.ListFormat type declarations Dec 26, 2021
andreubotella pushed a commit to andreubotella/deno that referenced this pull request Jan 7, 2022
V8 has supported `Intl.ListFormat` since version 7.2, but TypeScript doesn't
have typings for it yet. This PR manually adds those typings, copying them from
microsoft/TypeScript#47254.

Fixes denoland#13199.
bartlomieju pushed a commit to denoland/deno that referenced this pull request Jan 17, 2022
V8 has supported `Intl.ListFormat` since version 7.2, but TypeScript doesn't
have typings for it yet. This PR manually adds those typings, copying them from
microsoft/TypeScript#47254.
@sandersn sandersn self-assigned this Feb 16, 2022
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Seems correct, though you probably need the aforementioned | undefined.

src/lib/es2021.intl.d.ts Outdated Show resolved Hide resolved
src/lib/es2021.intl.d.ts Outdated Show resolved Hide resolved
src/lib/es2021.intl.d.ts Show resolved Hide resolved
*
* [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat/ListFormat#parameters).
*/
type ListFormatLocaleMatcher = "lookup" | "best fit";
Copy link
Member

Choose a reason for hiding this comment

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

One final comment: I'm noticing that none of the options above have defined types (or make use of the new ones); is that intentional? Is it more "correct" to not have defined types for any of them? Or do they need to be fixed to have defined types too?

(I'm also noticing that there are some silly things above like "best fit" | "best fit".)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not touch it in this PR, but create a follow-up PR in the next days instead.

  • Create named types for the options above, remove duplicate "best fit"
  • Investigate if ResolvedDateTimeFormatOptions is even correct (at a first glance, it seems wrong, but might be to match some unique browser behavior?)

Feel free to disagree, I can also tackle them here

Copy link
Member

Choose a reason for hiding this comment

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

I'm sure a second PR is fine, I just wasn't sure (I have no major knowledge of these types) whether or not it's normal to have top level declared type names or not.

@jakebailey jakebailey self-requested a review April 4, 2022 19:28
@DanielRosenwasser DanielRosenwasser merged commit 312737b into microsoft:main Apr 5, 2022
@predetermined predetermined deleted the feat/46907 branch April 5, 2022 20:49
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.

Add Intl.ListFormat to es2021 lib
6 participants