fix: Intl.NumberFormat locale RangeError related to Simulation Details #24068
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
dbrans
added
the
team-confirmations-system
dbrans
force-pushed
the
dbrans/locale-fix
branch
from
6592878 to
ca65bb1
Compare
dbrans
force-pushed
the
dbrans/locale-fix
branch
from
ca65bb1 to
56b2b72
Compare
ui/ducks/locale/locale.js
Outdated
| * @returns {string} one of the codes in file://./../../../app/_locales/index.json. | ||
| * These codes are not safe to use with the Intl API. | ||
| */ | ||
| export const getLocaleNotSafeForIntl = (state) => |
There was a problem hiding this comment.
For the sake of minimising refactor and the PR diff, is this really needed if SimulationDetails was the first usage of Intl.NumberFormat with one of these locales, and there was no issues previously?
Would the above warning in the JSDoc, plus the presence of the getIntlLocale method be a sufficient deterrent?
ui/ducks/locale/locale.js
Outdated
| export const getCurrentLocale = (state) => state.localeMessages.currentLocale; | ||
| /** | ||
| * @param state | ||
| * @returns {string} one of the codes in file://./../../../app/_locales/index.json. |
There was a problem hiding this comment.
Should we be explicit in both these method's JSDoc that it's the user's selected locale that is returned?
ui/ducks/locale/locale.test.ts
Outdated
| }); | ||
|
|
||
| describe('getIntlLocale', () => { | ||
| it.each(locales)('can convert locale "%s" to BCP 47 format', (locale) => { |
There was a problem hiding this comment.
Love this test! But if this is the happy path, should we also have a unit test to verify we throw if a bad locale is in the state?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #24068 +/- ##
===========================================
+ Coverage 67.56% 67.58% +0.02%
===========================================
Files 1246 1247 +1
Lines 48888 48938 +50
Branches 12746 12775 +29
===========================================
+ Hits 33031 33073 +42
- Misses 15857 15865 +8 ☔ View full report in Codecov by Sentry. |
metamaskbot
added
the
release-11.16.0
Description
Problem: The locale returned by
getCurrentLocaleis not meant to be used with the Intl API.To fix this, this PR will:
getCurrentLocale=>getLocaleNotSafeForIntlto alert developers in the future.getIntlLocalewhich converts the locale returned fromgetLocaleNotSafeForIntlinto a BCP 47 Language Tag which can be used with the Intl API.getIntlLocaleworks with all our locales going forward.Related issues
Fixes: #24067
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist