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

fix: Intl.NumberFormat locale RangeError related to Simulation Details #24068

Merged
merged 2 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ui/components/app/nfts-tab/nfts-tab.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {
MetaMetricsEventCategory,
MetaMetricsEventName,
} from '../../../../shared/constants/metametrics';
import { getCurrentLocale } from '../../../ducks/locale/locale';
import { getLocaleNotSafeForIntl } from '../../../ducks/locale/locale';
///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
import {
RAMPS_CARD_VARIANT_TYPES,
Expand Down Expand Up @@ -89,7 +89,7 @@ export default function NftsTab() {
const hasAnyNfts = Object.keys(collections).length > 0;
const showNftBanner = hasAnyNfts === false;
const { chainId, nickname } = useSelector(getCurrentNetwork);
const currentLocale = useSelector(getCurrentLocale);
const currentLocale = useSelector(getLocaleNotSafeForIntl);

useEffect(() => {
if (!showNftBanner) {
Expand Down
4 changes: 2 additions & 2 deletions ui/components/app/whats-new-popup/whats-new-popup.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
} from '../../../../shared/notifications';
import { I18nContext } from '../../../contexts/i18n';
import { MetaMetricsContext } from '../../../contexts/metametrics';
import { getCurrentLocale } from '../../../ducks/locale/locale';
import { getLocaleNotSafeForIntl } from '../../../ducks/locale/locale';
import { TextVariant } from '../../../helpers/constants/design-system';
import { ADVANCED_ROUTE } from '../../../helpers/constants/routes';
import ZENDESK_URLS from '../../../helpers/constants/zendesk-url';
Expand Down Expand Up @@ -211,7 +211,7 @@ export default function WhatsNewPopup({ onClose }) {
const history = useHistory();

const notifications = useSelector(getSortedAnnouncementsToShow);
const locale = useSelector(getCurrentLocale);
const locale = useSelector(getLocaleNotSafeForIntl);

///: BEGIN:ONLY_INCLUDE_IF(blockaid)
const theme = useTheme();
Expand Down
4 changes: 2 additions & 2 deletions ui/components/multichain/ramps-card/ramps-card.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import useRamps, {
RampsMetaMaskEntry,
} from '../../../hooks/experiences/useRamps';
import { ORIGIN_METAMASK } from '../../../../shared/constants/app';
import { getCurrentLocale } from '../../../ducks/locale/locale';
import { getLocaleNotSafeForIntl } from '../../../ducks/locale/locale';

const darkenGradient =
'linear-gradient(rgba(0, 0, 0, 0.12),rgba(0, 0, 0, 0.12))';
Expand Down Expand Up @@ -67,7 +67,7 @@ export const RampsCard = ({ variant }) => {
RAMPS_CARD_VARIANTS[variant];
const { openBuyCryptoInPdapp } = useRamps(metamaskEntryMap[variant]);
const trackEvent = useContext(MetaMetricsContext);
const currentLocale = useSelector(getCurrentLocale);
const currentLocale = useSelector(getLocaleNotSafeForIntl);
const { chainId, nickname } = useSelector(getCurrentNetwork);
const { symbol = 'ETH' } = useSelector(getSwapsDefaultToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ import {
import { getCurrentNetwork, getSelectedAccount } from '../../../selectors';
import { ReceiveModal } from '../receive-modal';
import { ORIGIN_METAMASK } from '../../../../shared/constants/app';
import { getCurrentLocale } from '../../../ducks/locale/locale';
import { getLocaleNotSafeForIntl } from '../../../ducks/locale/locale';

export const ReceiveTokenLink: React.FC<BoxProps<'div'>> = ({
...props
}): JSX.Element => {
const trackEvent = useContext(MetaMetricsContext);
const t = useI18nContext();
const currentNetwork = useSelector(getCurrentNetwork);
const currentLocale = useSelector(getCurrentLocale);
const currentLocale = useSelector(getLocaleNotSafeForIntl);
const { address: selectedAddress } = useSelector(getSelectedAccount);

const [showReceiveModal, setShowReceiveModal] = useState(false);
Expand Down
4 changes: 2 additions & 2 deletions ui/contexts/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React, { Component, createContext, useMemo } from 'react';
import PropTypes from 'prop-types';
import { useSelector } from 'react-redux';
import {
getCurrentLocale,
getLocaleNotSafeForIntl,
getCurrentLocaleMessages,
getEnLocaleMessages,
} from '../ducks/locale/locale';
Expand All @@ -11,7 +11,7 @@ import { getMessage } from '../helpers/utils/i18n-helper';
export const I18nContext = createContext((key) => `[${key}]`);

export const I18nProvider = (props) => {
const currentLocale = useSelector(getCurrentLocale);
const currentLocale = useSelector(getLocaleNotSafeForIntl);
const current = useSelector(getCurrentLocaleMessages);
const en = useSelector(getEnLocaleMessages);

Expand Down
21 changes: 20 additions & 1 deletion 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,7 +14,25 @@ export default function reduceLocaleMessages(state = {}, { type, payload }) {
}
}

export const getCurrentLocale = (state) => state.localeMessages.currentLocale;
/**
* @param state
* @returns {string} one of the codes in file://./../../../app/_locales/index.json.
Copy link
Member

Choose a reason for hiding this comment

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

Should we be explicit in both these method's JSDoc that it's the user's selected locale that is returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* These codes are not safe to use with the Intl API.
*/
export const getLocaleNotSafeForIntl = (state) =>
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

state.localeMessages.currentLocale;

/**
* This selector returns a code from /app/_locales/index.json as a
* [BCP 47 Language Tag](https://en.wikipedia.org/wiki/IETF_language_tag) for use with
* the Intl API.
*
* @returns {Intl.UnicodeBCP47LocaleIdentifier} a locale code that can be used with the Intl API
*/
export const getIntlLocale = createSelector(
getLocaleNotSafeForIntl,
(locale) => Intl.getCanonicalLocales(locale.replace(/_/gu, '-'))[0],
);

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

Expand Down
15 changes: 15 additions & 0 deletions ui/ducks/locale/locale.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import locales from '../../../app/_locales/index.json';
import { getIntlLocale } from './locale';

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

describe('getIntlLocale', () => {
it.each(locales)('can convert locale "%s" to BCP 47 format', (locale) => {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done.

const mockState = createMockStateWithLocale(locale.code);

// Intl.getCanonicalLocales will throw an error if the locale is invalid.
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ import React from 'react';
import { useDispatch, useSelector } from 'react-redux';
import MetaFoxLogo from '../../../components/ui/metafox-logo';
import Dropdown from '../../../components/ui/dropdown';
import { getCurrentLocale } from '../../../ducks/locale/locale';
import { getLocaleNotSafeForIntl } from '../../../ducks/locale/locale';
import { updateCurrentLocale } from '../../../store/actions';
import locales from '../../../../app/_locales/index.json';

export default function OnboardingAppHeader() {
const dispatch = useDispatch();
const currentLocale = useSelector(getCurrentLocale);
const currentLocale = useSelector(getLocaleNotSafeForIntl);
const localeOptions = locales.map((locale) => {
return {
name: locale.name,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
import { useI18nContext } from '../../../hooks/useI18nContext';
import { MetaMetricsContext } from '../../../contexts/metametrics';
import { ONBOARDING_REVIEW_SRP_ROUTE } from '../../../helpers/constants/routes';
import { getCurrentLocale } from '../../../ducks/locale/locale';
import { getLocaleNotSafeForIntl } from '../../../ducks/locale/locale';
import {
MetaMetricsEventCategory,
MetaMetricsEventName,
Expand All @@ -38,7 +38,7 @@ export default function SecureYourWallet() {
const history = useHistory();
const t = useI18nContext();
const { search } = useLocation();
const currentLocale = useSelector(getCurrentLocale);
const currentLocale = useSelector(getLocaleNotSafeForIntl);
const [showSkipSRPBackupPopover, setShowSkipSRPBackupPopover] =
useState(false);
const searchParams = new URLSearchParams(search);
Expand Down
Loading