From 5363d6aa2a649a2408db7e930b4a7c93264886eb Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 6 Sep 2019 10:47:07 -0300 Subject: [PATCH] Prevent multiple warnings for the same locale message (#7091) * Prevent multiple warnings for the same locale message Our i18n helper issues warnings whenever a requested message is missing. These warnings are helpful in assisting translation efforts, but they can be distracting otherwise. They're especially problematic for locales that are missing many messages. My browser ended up crashing on more than one occasion due to the sheer volume of warnings. The warning has been updated to only be issued once per missing key. This required updating the method to pass in the current locale. The current locale was added to the warning itself as well, which could be helpful for cases where a message is missing in both the current locale and the fallback ('en'). * Change locale and localeMessages as single action Updating the current locale and the locale messages resulted in two renders, and during the first the state was inconsistent (it would say the locale had changed to the new one, but still be using the old set of locale messages). Instead the locale is now updated with one atomic action. This was required after adding the locale to the missing locale message warning, as otherwise it would say the message was missing from the wrong locale. --- test/unit/ui/app/actions.spec.js | 3 +-- test/unit/ui/app/reducers/metamask.spec.js | 2 +- ui/app/ducks/locale/locale.js | 4 +-- ui/app/ducks/metamask/metamask.js | 2 +- .../higher-order-components/i18n-provider.js | 12 +++++---- ui/app/helpers/utils/i18n-helper.js | 26 ++++++++++++------- ui/app/store/actions.js | 19 +++++--------- 7 files changed, 35 insertions(+), 33 deletions(-) diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js index 33b223c62..2ed35bc5b 100644 --- a/test/unit/ui/app/actions.spec.js +++ b/test/unit/ui/app/actions.spec.js @@ -1155,9 +1155,8 @@ describe('Actions', () => { const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, + { type: 'SET_CURRENT_LOCALE', value: { locale: 'en', messages: enLocale }}, { type: 'HIDE_LOADING_INDICATION' }, - { type: 'SET_CURRENT_LOCALE', value: 'en' }, - { type: 'SET_LOCALE_MESSAGES', value: enLocale }, ] return store.dispatch(actions.updateCurrentLocale('en')) diff --git a/test/unit/ui/app/reducers/metamask.spec.js b/test/unit/ui/app/reducers/metamask.spec.js index 714bd476a..6b3dd7193 100644 --- a/test/unit/ui/app/reducers/metamask.spec.js +++ b/test/unit/ui/app/reducers/metamask.spec.js @@ -456,7 +456,7 @@ describe('MetaMask Reducers', () => { it('sets current locale', () => { const state = reduceMetamask({}, { type: actions.SET_CURRENT_LOCALE, - value: 'ge', + value: { locale: 'ge' }, }) assert.equal(state.currentLocale, 'ge') diff --git a/ui/app/ducks/locale/locale.js b/ui/app/ducks/locale/locale.js index bb8e1b08e..747e227ec 100644 --- a/ui/app/ducks/locale/locale.js +++ b/ui/app/ducks/locale/locale.js @@ -7,9 +7,9 @@ function reduceMetamask (state, action) { const localeMessagesState = extend({}, state.localeMessages) switch (action.type) { - case actions.SET_LOCALE_MESSAGES: + case actions.SET_CURRENT_LOCALE: return extend(localeMessagesState, { - current: action.value, + current: action.value.messages, }) default: return localeMessagesState diff --git a/ui/app/ducks/metamask/metamask.js b/ui/app/ducks/metamask/metamask.js index 35de947b4..f1fa7cd03 100644 --- a/ui/app/ducks/metamask/metamask.js +++ b/ui/app/ducks/metamask/metamask.js @@ -377,7 +377,7 @@ function reduceMetamask (state, action) { case actions.SET_CURRENT_LOCALE: return extend(metamaskState, { - currentLocale: action.value, + currentLocale: action.value.locale, }) case actions.SET_PENDING_TOKENS: diff --git a/ui/app/helpers/higher-order-components/i18n-provider.js b/ui/app/helpers/higher-order-components/i18n-provider.js index 5a6650147..1360cf5fd 100644 --- a/ui/app/helpers/higher-order-components/i18n-provider.js +++ b/ui/app/helpers/higher-order-components/i18n-provider.js @@ -7,12 +7,12 @@ const t = require('../utils/i18n-helper').getMessage class I18nProvider extends Component { tOrDefault = (key, defaultValue, ...args) => { - const { localeMessages: { current, en } = {} } = this.props - return t(current, key, ...args) || t(en, key, ...args) || defaultValue + const { localeMessages: { current, en } = {}, currentLocale } = this.props + return t(currentLocale, current, key, ...args) || t(currentLocale, en, key, ...args) || defaultValue } getChildContext () { - const { localeMessages } = this.props + const { localeMessages, currentLocale } = this.props const { current, en } = localeMessages return { /** @@ -26,7 +26,7 @@ class I18nProvider extends Component { return key } - return t(current, key, ...args) || t(en, key, ...args) || `[${key}]` + return t(currentLocale, current, key, ...args) || t(currentLocale, en, key, ...args) || `[${key}]` }, tOrDefault: this.tOrDefault, tOrKey: (key, ...args) => { @@ -42,6 +42,7 @@ class I18nProvider extends Component { I18nProvider.propTypes = { localeMessages: PropTypes.object, + currentLocale: PropTypes.string, children: PropTypes.object, } @@ -52,8 +53,9 @@ I18nProvider.childContextTypes = { } const mapStateToProps = state => { - const { localeMessages } = state + const { localeMessages, metamask: { currentLocale } } = state return { + currentLocale, localeMessages, } } diff --git a/ui/app/helpers/utils/i18n-helper.js b/ui/app/helpers/utils/i18n-helper.js index db07049e1..b9720acc7 100644 --- a/ui/app/helpers/utils/i18n-helper.js +++ b/ui/app/helpers/utils/i18n-helper.js @@ -1,22 +1,30 @@ // cross-browser connection to extension i18n API const log = require('loglevel') +const warned = {} /** * Returns a localized message for the given key - * @param {object} locale The locale + * @param {string} localeCode The code for the current locale + * @param {object} localeMessages The map of messages for the current locale * @param {string} key The message key * @param {string[]} substitutions A list of message substitution replacements * @return {null|string} The localized message */ -const getMessage = (locale, key, substitutions) => { - if (!locale) { +const getMessage = (localeCode, localeMessages, key, substitutions) => { + if (!localeMessages) { return null } - if (!locale[key]) { - log.warn(`Translator - Unable to find value for key "${key}"`) + if (!localeMessages[key]) { + if (!warned[localeCode] || !warned[localeCode][key]) { + if (!warned[localeCode]) { + warned[localeCode] = {} + } + warned[localeCode][key] = true + log.warn(`Translator - Unable to find value of key "${key}" for locale "${localeCode}"`) + } return null } - const entry = locale[key] + const entry = localeMessages[key] let phrase = entry.message // perform substitutions if (substitutions && substitutions.length) { @@ -28,12 +36,12 @@ const getMessage = (locale, key, substitutions) => { return phrase } -async function fetchLocale (localeName) { +async function fetchLocale (localeCode) { try { - const response = await fetch(`./_locales/${localeName}/messages.json`) + const response = await fetch(`./_locales/${localeCode}/messages.json`) return await response.json() } catch (error) { - log.error(`failed to fetch ${localeName} locale because of ${error}`) + log.error(`failed to fetch ${localeCode} locale because of ${error}`) return {} } } diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index 87b01a74c..1b393d83a 100644 --- a/ui/app/store/actions.js +++ b/ui/app/store/actions.js @@ -308,10 +308,8 @@ var actions = { // locale SET_CURRENT_LOCALE: 'SET_CURRENT_LOCALE', - SET_LOCALE_MESSAGES: 'SET_LOCALE_MESSAGES', setCurrentLocale, updateCurrentLocale, - setLocaleMessages, // // Feature Flags setFeatureFlag, @@ -2604,25 +2602,20 @@ function updateCurrentLocale (key) { return dispatch(actions.displayWarning(err.message)) } switchDirection(textDirection) + dispatch(actions.setCurrentLocale(key, localeMessages)) dispatch(actions.hideLoadingIndication()) - dispatch(actions.setCurrentLocale(key)) - dispatch(actions.setLocaleMessages(localeMessages)) }) }) } } -function setCurrentLocale (key) { +function setCurrentLocale (locale, messages) { return { type: actions.SET_CURRENT_LOCALE, - value: key, - } -} - -function setLocaleMessages (localeMessages) { - return { - type: actions.SET_LOCALE_MESSAGES, - value: localeMessages, + value: { + locale, + messages, + }, } }