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

Stricter type for ES5 Intl, redefine ES2020 intl #42134

Merged
merged 1 commit into from
Dec 28, 2020

Conversation

batisteo
Copy link
Contributor

Fixes #42128

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 28, 2020
@batisteo batisteo marked this pull request as ready for review December 28, 2020 12:45
@orta orta self-assigned this Dec 28, 2020
@orta
Copy link
Contributor

orta commented Dec 28, 2020

Thanks! This looks good to me 👍🏻

@orta orta merged commit e4c4292 into microsoft:master Dec 28, 2020
Zzzen pushed a commit to Zzzen/TypeScript that referenced this pull request Jan 16, 2021
@longlho
Copy link
Contributor

longlho commented Feb 17, 2021

@orta this should be revised/reverted. List of numbering system & calendar are NOT part of the ECMA402 spec. They conform to Unicode Locale Identifier semantic restriction, but the list can change every CLDR version which is separate from ECMA402.

Reference of failure upstream: formatjs/formatjs#2616

@nstepien
Copy link

@longlho Can't TS keep up with CLDR versions regardless?

@longlho
Copy link
Contributor

longlho commented Feb 17, 2021

no because every browser version potentially ships with a different CLDR/libicu version. The release cadence is different. The spec also intentionally does not rely on this except for things like:

An implementation can be spec-compliant if shipped with a newer/older CLDR version.

@RyanCavanaugh
Copy link
Member

@longlho I'm not seeing why this needs a revert. We can update this file out-of-band to include whatever the largest current list is without loss of correctness relative to just string. Can you clarify?

@longlho
Copy link
Contributor

longlho commented Feb 17, 2021

So the problem is I can ship an implementation with calendar: foo and that's totally spec-compliant. An implementation is not required to implement all or subset of those calendars. ECMA402 is very intentional on what it relies on from Unicode and what's not.

Listing it here in TS suggests that those are only valid values and others are not while in reality, it is indeed just a string of a specific semantic and API user should validate against calendar support.

@longlho
Copy link
Contributor

longlho commented Feb 17, 2021

The es5 change is ok though, because the spec dictates that if you pass in an invalid option like 3-digit instead of 2-digit it will throw a RangeError.

@RyanCavanaugh
Copy link
Member

So the problem is I can ship an implementation with calendar: foo and that's totally spec-compliant.

Make sense. From a practical perspective, is this a thing that happens with regularity, or is it worth catching a subtle typo like "ethiopian" for the sake of making someone write "foo" as any ?

@longlho
Copy link
Contributor

longlho commented Feb 17, 2021

Hmm I can see the argument for typo but I think at the end of the day you'd still need to manually do like isCalendarSupported('ethiopia') regardless and that should determine runtime capability. Besides, calendar names kinda... looks like typo (e.g ethioaa is shorthand for ethiopic-amete-alem. It used to be called ethiopia in CLDRv82 but changed to ethioaa in CLDRv83).

@RyanCavanaugh
Copy link
Member

This has been merged for six weeks without visible complaint from user code, so I'm willing to let this go out in 4.2 and revert if we find people in the wild hitting it if the problem is that it's only possibly-too-strict on an input side.

@longlho
Copy link
Contributor

longlho commented Feb 17, 2021

Does it help if I just make a PR to revert it?
This is neither here nor there because the reality is people rarely deal with non-gregory calendar & non-latin numbering system so you won't get direct complaints other than from formatjs which powers all the ECMA402 polyfills 😢 Our users won't be able to compile w/ TS4.2 basically (~600K downloads/week) (See formatjs/formatjs#2616 as a reference).

In the future I'm happy to review changes like these if that helps.

@nstepien
Copy link

@longlho Why is it ok for formatjs to use an enum there, but not TS? 🤔

@longlho
Copy link
Contributor

longlho commented Feb 17, 2021

because that's the set of stuff we specifically support :) so if you're using formatjs you only get access to those calendar/numbering systems. If you use a browser's non-polyfilled Intl then it's unknown.

@nstepien
Copy link

Right, so an alternative fix would be for formatjs to do this:

-export interface DateTimeFormatOptions extends Intl.DateTimeFormatOptions {
+export interface DateTimeFormatOptions extends Omit<Intl.DateTimeFormatOptions, 'calendar'> {

@longlho
Copy link
Contributor

longlho commented Feb 17, 2021

Those are 2 separate problems though. I'm saying TS provides low level type def for ECMA402 API and right now it's in unspec'ed land with calendar & numberingSystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lib.es2020.intl.d.ts conflicts with lib.es5.d.ts
6 participants