Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Fix typescript error in i18n #2821

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Fix typescript error in i18n #2821

merged 2 commits into from
Aug 28, 2024

Conversation

alex-page
Copy link
Member

@alex-page alex-page commented Aug 21, 2024

Updated NumberFormatOptionsStyleRegistry to align with latest version of typescript.

@alex-page alex-page marked this pull request as ready for review August 28, 2024 00:16
@alex-page alex-page requested a review from a team as a code owner August 28, 2024 00:16
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Can we update this to as?: 'decimal' | 'currency' | 'percent'; so we fix the single problematic value, rather than widening the set of allowed values to include the es2020+ only 'unit'.


In TS 5.4 Intl.NumberFormatOptions['style'] was string | undefined.

TS 5.5 strictens that up to be a set of possible values (keyof Intl.NumberFormatOptionsStyleRegistry, the exact values of which change depending on which es version you're targeting - es5 supports decimal, percent and currency. es2020 adds unit).

No idea why /snapit is installing TS 5.5 when we request ts 5.4 in the package.json though.


I'm not sure if we want to count this as a patch or major.

We're removing a "valid" value which is technically a breaking change

But MDN suggests that number was never a valid value for the style option in the first place and doing Intl.NumberFormat('en', {style: 'number'}) is a runtime error. So we're making what was a runtime error now be highlighted at type-check time.

I'm tempted to try this as a patch change, do a snapit and see what breaks in web - if that's all good then do a patch.

@alex-page alex-page force-pushed the react-i18n-ts-error branch 3 times, most recently from 45ab866 to 8e7d60b Compare August 28, 2024 00:42
@alex-page
Copy link
Member Author

/snapit

Copy link
Contributor

🫰✨ Thanks @alex-page! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

"@shopify/react-i18n": "0.0.0-snapshot-20240828004508",
"@shopify/react-i18n-universal-provider": "0.0.0-snapshot-20240828004508"

@alex-page
Copy link
Member Author

alex-page commented Aug 28, 2024

Tophatted in /web in https://github.com/Shopify/web/pull/138778

There were some issues but they can be fixed forward in /web. Nothing critical from what I could see quickly.

@alex-page alex-page merged commit 8c53994 into main Aug 28, 2024
5 checks passed
@alex-page alex-page deleted the react-i18n-ts-error branch August 28, 2024 01:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants