Skip to content

fix: fix RelativeTimeFormat type definition #39661

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 2 commits into from
Feb 16, 2021
Merged

fix: fix RelativeTimeFormat type definition #39661

merged 2 commits into from
Feb 16, 2021

Conversation

longlho
Copy link
Contributor

@longlho longlho commented Jul 19, 2020 •
edited
Loading

Changes:

  1. Change BCP47LanguageTag to UnicodeBCP47LocaleIdentifier: Those mean 2
    different things. BCP47LangTag allows _ as separator while UTS35
    doesn't. It also allows grandfathered locales and UTS35 doesn't.

Fixes #39660

@longlho
fix: fix RelativeTimeFormat type definition
8d44809 
Changes:
1. Change BCP47LanguageTag to UnicodeBCP47LocaleIdentifier: Those mean 2
different things. BCP47LangTag allows _ as separator while UTS35
doesn't. It also allows grandfathered locales and UTS35 doesn't.

2. Combine RelativeTimeFormat interface and const declaration into a
single class. The old way of declaring as `interface` & `const` permits
calling `Intl.RelativeTimeFormat` without `new` which is no longer
possible after `Intl.DateTimeFormat` & `Intl.NumberFormat`. The spec
explicitly forbids it in
http://ecma-international.org/ecma-402/7.0/index.html#relativetimeformat-objects
where:

> If NewTarget is undefined, throw a TypeError exception.

Intl.RelativeTimeFormat is also extensible per spec. This is closer to a
`class` than the current declaration.
@longlho
Copy link
Contributor Author

longlho commented Jul 19, 2020

cc @orta

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jul 20, 2020
orta
orta approved these changes Jul 20, 2020
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.

Cool, thanks, this looks good to me.

I don't think it's a breaking change given that it moved from a const with new() to a class, and given that at runtime it would throw. I'm good to merge, I'll give it a day or two for others to take a look over.

@orta orta self-assigned this Jul 20, 2020
@orta orta mentioned this pull request Jul 21, 2020
Closed
sandersn
sandersn requested changes Sep 3, 2020
View reviewed changes
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.

The type name change is good, but we should stick with the interface+const combo because it allows declaration merging.

@longlho
Copy link
Contributor Author

longlho commented Sep 18, 2020

gentle ping @sandersn

@sandersn sandersn self-requested a review February 16, 2021 23:56
@sandersn sandersn merged commit 3910d9e into microsoft:master Feb 16, 2021
@longlho longlho deleted the rtf branch February 17, 2021 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orta orta orta approved these changes

@sandersn sandersn sandersn approved these changes

Assignees

@orta orta

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.

Intl.RelativeTimeFormat does not correctly reflect the spec
4 participants
@longlho @orta @sandersn @typescript-bot