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

Intl.NumberFormat: missing options, and discussion on typing conventions #56269

Closed
Renegade334 opened this issue Oct 31, 2023 · 6 comments · Fixed by #56902
Closed

Intl.NumberFormat: missing options, and discussion on typing conventions #56269

Renegade334 opened this issue Oct 31, 2023 · 6 comments · Fixed by #56902
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@Renegade334
Copy link
Contributor

Renegade334 commented Oct 31, 2023

⚙ Compilation target

ESNext

⚙ Library

intl

Missing / Incorrect Definition

This was originally just going to highlight some missing NumberFormat definitions, but after a trawl through the library, rather grew in scope a bit.

I've therefore divided this issue down into three parts: two are details of changes that need to be made to the library, but the third is an RFD that arose out of inconsistencies in the Intl library definitions. If the project team feel this would be better forked elsewhere, then feel free.

1: Properties of NumberFormatOptions/ResolvedNumberFormatOptions diverging from previous specifications

es5.d.ts – ECMA-402 1.0 (2012)
  • currencySign is defined as a property here, but does not exist in the specification
  • currencyDisplay is not defined as a property here, but exists in the specification as "code" | "symbol" | "name" | undefined
es2020.intl.d.ts – ECMA-402 7.0 (2020)
  • currencyDisplay should be removed (as should have been defined above)
  • numberingSystem is not defined as a property of NumberFormatOptions here, but exists in the specification as string | undefined

2: NumberFormatOptions changes in ECMA-402 10.0 (2023)

es2023.intl.d.ts should ideally be created to house these changes.

  • breaking changes from es2020.intl.d.ts:
    - useGrouping?: boolean | undefined;
    + useGrouping?: "min2" | "auto" | "always" | boolean | undefined;
    - signDisplay?: "auto" | "never" | "always" | "exceptZero" | undefined;
    + signDisplay?: "auto" | "never" | "always" | "exceptZero" | "negative" | undefined;
  • the following new properties have been added in the specification:
    roundingPriority?: "auto" | "morePrecision" | "lessPrecision" | undefined;
    roundingIncrement?: 1 | 2 | 5 | 10 | 20 | 25 | 50 | 100 | 200 | 250 | 500 | 1000 | 2000 | 2500 | 5000 | undefined;
    roundingMode?: "ceil" | "floor" | "expand" | "trunc" | "halfCeil" | "halfFloor" | "halfExpand" | "halfTrunc" | "halfEven" | undefined;
    trailingZeroDisplay?: "auto" | "stripIfInteger" | undefined;
  • corresponding changes to ResolvedNumberFormatOptions:
    - useGrouping: boolean;
    + useGrouping: "min2" | "auto" | "always";
    - signDisplay?: "auto" | "never" | "always" | "exceptZero";
    + signDisplay?: "auto" | "never" | "always" | "exceptZero" | "negative";
    roundingMode: "ceil" | "floor" | "expand" | "trunc" | "halfCeil" | "halfFloor" | "halfExpand" | "halfTrunc" | "halfEven";
    roundingIncrement: 1 | 2 | 5 | 10 | 20 | 25 | 50 | 100 | 200 | 250 | 500 | 1000 | 2000 | 2500 | 5000;
    trailingZeroDisplay: "auto" | "stripIfInteger";

 

3: Discussion – primitives versus const unions for enum-like properties

There are a lot of properties in the Intl specification that accept an enum-like list of values as valid input.

There is no set standard in the library for how these are defined.

  • es5.d.ts tended to keep types as wide as possible, such as style?: string | undefined
  • subsequent library updates tended to narrow enum-like types to const unions, such as compactDisplay?: "short" | "long" | undefined

The latter has obvious advantages in terms of developer usability. However, it has a significant drawback, in that these enum-like properties have had a tendency to be modified fairly frequently in recent versions of the specification.

When this occurs to properties defined using primitive types, this isn't an issue for developers, as the change is non-breaking.
For example, the style property was defined in the original specification as "decimal" | "percent" | "currency" | undefined, but was added to the library as string | undefined; as such, when "unit" was added as a style in ES2020, the change went unnoticed.

However, when this occurs to properties defined as const unions, such as with a couple of the examples above, it's a different story.

  • Firstly, the new property values can't be used until they're added into the library, despite the primitive type of the property not having changed.
  • Secondly, making the changes to the library is going to have to involve some sort of workaround, as modifying the type of an existing interface property is obviously not possible.

As such, I think the project needs to decide on a couple of conventions:

  1. Should enum-like properties in the Intl library be typed as "const" | "unions" | "where" | "possible", or should they be widened to their equivalent primitive?
  2. If the former is chosen, then whenever an enum-like property is modified in the specification, this will necessitate modifying existing interface properties in the Intl library; what is the best way of implementing these changes?

Sample Code

No response

Documentation Link

No response

@RyanCavanaugh
Copy link
Member

The original version of es5.d.ts was written before literal types existed, so that's just an artifact of history / not wanting to do somewhat arbitrary breaking changes.

to be modified fairly frequently in recent versions of the specification.

Usually there's easily enough time between when these things get added to the spec and when the last major browser (cough Safari) implements it, so in practice it's not a huge deal.

Firstly, the new property values can't be used until they're added into the library

I mean, this is always true of anything. Today Array doesn't have a removeDuplicates() function, but it might tomorrow. If that fact changes, the lib will change too. I don't understand how this is any different.

whenever an enum-like property is modified in the specification

We should edit the lib when this happens. I'm not sure what else is even on the table?

Regarding currencySign etc, PRs welcomed once the sample code / doc link section is filled out

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Help Wanted You can do this Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels Oct 31, 2023
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Oct 31, 2023
@Renegade334
Copy link
Contributor Author

Renegade334 commented Nov 2, 2023

The original version of es5.d.ts was written before literal types existed, so that's just an artifact of history / not wanting to do somewhat arbitrary breaking changes.

So would you prefer to keep things as-is, ie. have one typing convention for the historically defined properties and a different one for more recently added ones, to avoid any retrospective breaking?

One alternative would be to keep the original library declarations as-is, but in es2023.intl (or esnext.intl), change the previously "wide" property types to the const unions defined in the standard? That would bring everything in line, but projects built against previous ES targets would be unaffected.

We should edit the lib when this happens. I'm not sure what else is even on the table?

What method would you advocate for implementing this in the declaration files? The usual interface-merging technique isn't possible for editing existing properties, as the types for the old and new property are mismatched, and at a quick glance I can't find an obvious equivalent elsewhere in the library to go off.

I mean, this is always true of anything. Today Array doesn't have a removeDuplicates() function, but it might tomorrow. If that fact changes, the lib will change too. I don't understand how this is any different.

This case is slightly different as it involves type changes to existing interface properties, rather than adding new ones, so has the potential to be more "breaky". Maybe I'm just overthinking.

@WORMSS
Copy link

WORMSS commented Feb 4, 2024

Nice to know I am not the only one who has noticed these problems..
I have been able to compensate with most of the missing options, but the missing 'negative' from signDisplay has caused more headaches and I've had to resort to as which is annoying me greatly.

rajsite pushed a commit to ni/nimble that referenced this issue Feb 26, 2024
# Pull Request

## 🤨 Rationale

Fixes: #1442 

## 👩‍💻 Implementation

The options cited in the issue are:

- `useGrouping: 'auto'` - We previously were setting this to `true`,
which means "display grouping separators even if the locale prefers
otherwise." Instead we want `"auto"`, which is "display grouping
separators based on the locale preference." The default for this option
is `"auto"` unless `notation` is `"compact"`. We never set `notation` to
`"compact"`, so we can just leave `useGrouping` to use the default.
- `signDisplay: 'negative'` - Though this option value has been
[supported in all browsers since
8/23](https://caniuse.com/mdn-javascript_builtins_intl_numberformat_numberformat_options_signdisplay_parameter_negative),
there does not seem to be a version of Typescript whose definition of
`Intl.NumberFormatOptions` allows it. There is an [open issue for adding
support](microsoft/TypeScript#56269). As a
workaround, I've just added type assertions where necessary.
- `roundingPriority: 'lessPrecision'` - Just like `signDisplay:
'negative'`, the `roundingPriority` option has been [supported in
browsers since
8/23](https://caniuse.com/mdn-javascript_builtins_intl_numberformat_numberformat_options_roundingpriority_parameter),
but Typescript doesn't have support yet. It is tracked by the same issue
linked above, and our type assertion serves as a workaround for this,
too. By using this option, we can get rid of the separate
`leadingZeroFormatter` in `DefaultUnitFormat` and simplify the logic.

## 🧪 Testing

Existing tests pass.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@WORMSS
Copy link

WORMSS commented Mar 1, 2024

@sandersn Nice..
How is best to follow the progress of these changes from this point on until it hits a release?

@sandersn
Copy link
Member

sandersn commented Mar 1, 2024

#57475 maybe?

@WORMSS
Copy link

WORMSS commented Mar 1, 2024

#57475 maybe?

Thank you, subscribed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants