Skip to content

Commit

Permalink
fix: Intl.NumberFormat locale RangeError related to Simulation Details (
Browse files Browse the repository at this point in the history
#24068)

## **Description**
**Problem:** The locale returned by `getCurrentLocale` is not meant to
be used with the [Intl
API](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl).

**To fix this, this PR will:**
* Rename `getCurrentLocale` => `getLocaleNotSafeForIntl` to alert
developers in the future.
* Add `getIntlLocale` which converts the locale returned from
`getLocaleNotSafeForIntl` into a BCP 47 Language Tag which can be used
with the Intl API.
* Add tests to ensure that `getIntlLocale` works with all our locales
going forward.

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24068?quickstart=1)

## **Related issues**

Fixes: #24067

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
dbrans authored Apr 17, 2024
1 parent 915aa5b commit 15fc955
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 14 deletions.
22 changes: 22 additions & 0 deletions ui/ducks/locale/locale.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { createSelector } from 'reselect';
import * as actionConstants from '../../store/actionConstants';

export default function reduceLocaleMessages(state = {}, { type, payload }) {
Expand All @@ -13,8 +14,29 @@ export default function reduceLocaleMessages(state = {}, { type, payload }) {
}
}

/**
* This selector returns a code from file://./../../../app/_locales/index.json.
*
* NOT SAFE FOR INTL API USE. Use getIntlLocale instead for that.
*
* @param state
* @returns {string} the user's selected locale.
* These codes are not safe to use with the Intl API.
*/
export const getCurrentLocale = (state) => state.localeMessages.currentLocale;

/**
* This selector returns a
* [BCP 47 Language Tag](https://en.wikipedia.org/wiki/IETF_language_tag)
* for use with the Intl API.
*
* @returns {Intl.UnicodeBCP47LocaleIdentifier} the user's selected locale.
*/
export const getIntlLocale = createSelector(
getCurrentLocale,
(locale) => Intl.getCanonicalLocales(locale.replace(/_/gu, '-'))[0],
);

export const getCurrentLocaleMessages = (state) => state.localeMessages.current;

export const getEnLocaleMessages = (state) => state.localeMessages.en;
26 changes: 26 additions & 0 deletions ui/ducks/locale/locale.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import locales from '../../../app/_locales/index.json';
import { getIntlLocale } from './locale';

const createMockStateWithLocale = (locale: string) => ({
localeMessages: { currentLocale: locale },
});

describe('getIntlLocale', () => {
it('returns the canonical BCP 47 language tag for the currently selected locale', () => {
const mockState = createMockStateWithLocale('ab-cd');

expect(getIntlLocale(mockState)).toBe('ab-CD');
});

it('throws an error if locale cannot be made into BCP 47 language tag', () => {
const mockState = createMockStateWithLocale('xxxinvalid-locale');

expect(() => getIntlLocale(mockState)).toThrow();
});

it.each(locales)('handles all supported locales – "%s"', (locale) => {
const mockState = createMockStateWithLocale(locale.code);

expect(() => getIntlLocale(mockState)).not.toThrow();
});
});
19 changes: 7 additions & 12 deletions ui/hooks/useFiatFormatter.test.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,30 @@
import { renderHook } from '@testing-library/react-hooks';
import { getCurrentLocale } from '../ducks/locale/locale';
import { getIntlLocale } from '../ducks/locale/locale';
import { getCurrentCurrency } from '../selectors';
import { useFiatFormatter } from './useFiatFormatter';

// Mock the getCurrentLocale and getCurrentCurrency functions
jest.mock('react-redux', () => ({
useSelector: jest.fn((selector) => selector()),
}));

jest.mock('../ducks/locale/locale', () => ({
getCurrentLocale: jest.fn(),
getIntlLocale: jest.fn(),
}));

jest.mock('../selectors', () => ({
getCurrentCurrency: jest.fn(),
}));

const mockGetCurrentLocale = getCurrentLocale as jest.Mock;
const mockGetIntlLocale = getIntlLocale as unknown as jest.Mock;
const mockGetCurrentCurrency = getCurrentCurrency as jest.Mock;

describe('useFiatFormatter', () => {
beforeEach(() => {
// Clear the mock implementations before each test
mockGetCurrentLocale.mockClear();
mockGetCurrentCurrency.mockClear();
jest.clearAllMocks();
});

it('should return a function that formats fiat amount correctly', () => {
// Mock the getCurrentLocale and getCurrentCurrency functions
mockGetCurrentLocale.mockReturnValue('en-US');
mockGetIntlLocale.mockReturnValue('en-US');
mockGetCurrentCurrency.mockReturnValue('USD');

const { result } = renderHook(() => useFiatFormatter());
Expand All @@ -40,13 +36,12 @@ describe('useFiatFormatter', () => {
});

it('should use the current locale and currency from the mocked functions', () => {
// Mock the getCurrentLocale and getCurrentCurrency functions
mockGetCurrentLocale.mockReturnValue('fr-FR');
mockGetIntlLocale.mockReturnValue('fr-FR');
mockGetCurrentCurrency.mockReturnValue('EUR');

renderHook(() => useFiatFormatter());

expect(getCurrentLocale).toHaveBeenCalledTimes(1);
expect(getIntlLocale).toHaveBeenCalledTimes(1);
expect(getCurrentCurrency).toHaveBeenCalledTimes(1);
});
});
4 changes: 2 additions & 2 deletions ui/hooks/useFiatFormatter.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useSelector } from 'react-redux';
import { getCurrentLocale } from '../ducks/locale/locale';
import { getIntlLocale } from '../ducks/locale/locale';
import { getCurrentCurrency } from '../selectors';

/**
Expand All @@ -18,7 +18,7 @@ import { getCurrentCurrency } from '../selectors';
type FiatFormatter = (fiatAmount: number) => string;

export const useFiatFormatter = (): FiatFormatter => {
const locale = useSelector(getCurrentLocale);
const locale = useSelector(getIntlLocale);
const fiatCurrency = useSelector(getCurrentCurrency);

return (fiatAmount: number) => {
Expand Down

0 comments on commit 15fc955

Please sign in to comment.