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

[Audit][Implementation] - Reduce Redundant Operations #37200

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
60ea1a1
perf: remove redundant translations
hurali97 Feb 26, 2024
22f0ffa
perf: only parse the number if it matches the pattern
hurali97 Feb 26, 2024
643c7e1
perf: return early if displayName exists
hurali97 Feb 27, 2024
c683db7
test: fix failing test for formatPhoneNumber
hurali97 Feb 27, 2024
454a9b2
refactor: move translations to utils
hurali97 Feb 27, 2024
eb8f26c
fix: linting
hurali97 Feb 27, 2024
34cd2e8
refactor: use regex and dedicated const for sms domain
hurali97 Feb 28, 2024
707f56c
refactor: remove allocating a variable and return the evaluated expre…
hurali97 Feb 28, 2024
f36550f
refactor: use single-line comments
hurali97 Feb 29, 2024
e02b55a
refactor: use default policy room chat types from CONST
hurali97 Mar 4, 2024
7cbca08
revert: bring back localize.translateLocal
hurali97 Mar 4, 2024
4df1d3d
feat: add cache for translated values in Localize
hurali97 Mar 4, 2024
a7bd7d1
Merge branch 'main' of https://github.com/callstack-internal/Expensif…
hurali97 Mar 4, 2024
55eb5d2
refactor: delete common translation utils
hurali97 Mar 4, 2024
e643960
refactor: avoid multiple cache lookups
hurali97 Mar 4, 2024
f3d6f1f
fix: lint
hurali97 Mar 4, 2024
1755560
refactor: use dedicated map for each locale
hurali97 Mar 6, 2024
99dd34c
refactor: remove setImmediate
hurali97 Mar 6, 2024
3ff2f9c
refactor: remove not needed variable
hurali97 Mar 11, 2024
70bd88f
refactor: use Map of Maps to hold the locales with translated phrases
hurali97 Mar 11, 2024
9fc04db
refactor: move cache mechanism to translate function
hurali97 Mar 12, 2024
ce18f61
Merge branch 'main' of https://github.com/callstack-internal/Expensif…
hurali97 Mar 14, 2024
e22c22b
refactor: use dedicated function to get translated phrase to avoid du…
hurali97 Mar 14, 2024
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
20 changes: 12 additions & 8 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,21 @@ const keyInputRightArrow = KeyCommand?.constants?.keyInputRightArrow ?? 'keyInpu
// describes if a shortcut key can cause navigation
const KEYBOARD_SHORTCUT_NAVIGATION_TYPE = 'NAVIGATION_SHORTCUT';

const chatTypes = {
POLICY_ANNOUNCE: 'policyAnnounce',
POLICY_ADMINS: 'policyAdmins',
DOMAIN_ALL: 'domainAll',
POLICY_ROOM: 'policyRoom',
POLICY_EXPENSE_CHAT: 'policyExpenseChat',
SELF_DM: 'selfDM',
} as const;

// Explicit type annotation is required
const cardActiveStates: number[] = [2, 3, 4, 7];

const CONST = {
MERGED_ACCOUNT_PREFIX: 'MERGED_',
hurali97 marked this conversation as resolved.
Show resolved Hide resolved
DEFAULT_POLICY_ROOM_CHAT_TYPES: [chatTypes.POLICY_ADMINS, chatTypes.POLICY_ANNOUNCE, chatTypes.DOMAIN_ALL],
ANDROID_PACKAGE_NAME,
ANIMATED_TRANSITION: 300,
ANIMATED_TRANSITION_FROM_VALUE: 100,
Expand Down Expand Up @@ -704,14 +715,7 @@ const CONST = {
IOU: 'iou',
TASK: 'task',
},
CHAT_TYPE: {
POLICY_ANNOUNCE: 'policyAnnounce',
POLICY_ADMINS: 'policyAdmins',
DOMAIN_ALL: 'domainAll',
POLICY_ROOM: 'policyRoom',
POLICY_EXPENSE_CHAT: 'policyExpenseChat',
SELF_DM: 'selfDM',
},
CHAT_TYPE: chatTypes,
WORKSPACE_CHAT_ROOMS: {
ANNOUNCE: '#announce',
ADMINS: '#admins',
Expand Down
5 changes: 5 additions & 0 deletions src/libs/LocalePhoneNumber.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Str from 'expensify-common/lib/str';
import Onyx from 'react-native-onyx';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import {parsePhoneNumber} from './PhoneNumber';

Expand All @@ -18,6 +19,10 @@ function formatPhoneNumber(number: string): string {
return '';
}

// do not parse the string, if it doesn't contain the SMS domain and it's not a phone number
if (number.indexOf(CONST.SMS.DOMAIN) === -1 && !CONST.REGEX.DIGITS_AND_PLUS.test(number)) {
hurali97 marked this conversation as resolved.
Show resolved Hide resolved
return number;
}
const numberWithoutSMSDomain = Str.removeSMSDomain(number);
const parsedPhoneNumber = parsePhoneNumber(numberWithoutSMSDomain);

Expand Down
108 changes: 92 additions & 16 deletions src/libs/Localize/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import PropTypes from 'prop-types';
import * as RNLocalize from 'react-native-localize';
import Onyx from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import Log from '@libs/Log';
import type {MessageElementBase, MessageTextElement} from '@libs/MessageElement';
import Config from '@src/CONFIG';
Expand Down Expand Up @@ -50,34 +51,109 @@ type PhraseParameters<T> = T extends (...args: infer A) => string ? A : never[];
type Phrase<TKey extends TranslationPaths> = TranslationFlatObject[TKey] extends (...args: infer A) => unknown ? (...args: A) => string : string;

/**
* Return translated string for given locale and phrase
* Map to store translated values for each locale.
* This is used to avoid translating the same phrase multiple times.
*
* @param [desiredLanguage] eg 'en', 'es-ES'
* @param [phraseParameters] Parameters to supply if the phrase is a template literal.
* The data is stored in the following format:
*
* {
* "en": {
* "name": "Name",
* }
*
* Note: We are not storing any translated values for phrases with variables,
* as they have higher chance of being unique, so we'll end up wasting space
* in our cache.
*/
function translate<TKey extends TranslationPaths>(desiredLanguage: 'en' | 'es' | 'es-ES' | 'es_ES', phraseKey: TKey, ...phraseParameters: PhraseParameters<Phrase<TKey>>): string {
// Search phrase in full locale e.g. es-ES
const language = desiredLanguage === CONST.LOCALES.ES_ES_ONFIDO ? CONST.LOCALES.ES_ES : desiredLanguage;
let translatedPhrase = translations?.[language]?.[phraseKey] as Phrase<TKey>;
const translationCache = new Map<ValueOf<typeof CONST.LOCALES>, Map<TranslationPaths, string>>(
Object.values(CONST.LOCALES).reduce((cache, locale) => {
cache.push([locale, new Map()]);
return cache;
}, [] as Array<[ValueOf<typeof CONST.LOCALES>, Map<TranslationPaths, string>]>),
);

/**
* Helper function to get the translated string for given
* locale and phrase. This function is used to avoid
* duplicate code in getTranslatedPhrase and translate functions.
*
* This function first checks if the phrase is already translated
* and in the cache, it returns the translated value from the cache.
*
* If the phrase is not translated, it checks if the phrase is
* available in the given locale. If it is, it translates the
* phrase and stores the translated value in the cache and returns
* the translated value.
*
* @param language
* @param phraseKey
* @param fallbackLanguage
* @param phraseParameters
*/
function getTranslatedPhrase<TKey extends TranslationPaths>(
language: 'en' | 'es' | 'es-ES',
phraseKey: TKey,
fallbackLanguage: 'en' | 'es' | null = null,
...phraseParameters: PhraseParameters<Phrase<TKey>>
): string | null {
// Get the cache for the above locale
const cacheForLocale = translationCache.get(language);

// Directly access and assign the translated value from the cache, instead of
// going through map.has() and map.get() to avoid multiple lookups.
const valueFromCache = cacheForLocale?.get(phraseKey);

// If the phrase is already translated, return the translated value
if (valueFromCache) {
return valueFromCache;
}

const translatedPhrase = translations?.[language]?.[phraseKey] as Phrase<TKey>;

if (translatedPhrase) {
return typeof translatedPhrase === 'function' ? translatedPhrase(...phraseParameters) : translatedPhrase;
if (typeof translatedPhrase === 'function') {
return translatedPhrase(...phraseParameters);
}

// We set the translated value in the cache only for the phrases without parameters.
cacheForLocale?.set(phraseKey, translatedPhrase);
return translatedPhrase;
}

if (!fallbackLanguage) {
return null;
}

// Phrase is not found in full locale, search it in fallback language e.g. es
const languageAbbreviation = desiredLanguage.substring(0, 2) as 'en' | 'es';
translatedPhrase = translations?.[languageAbbreviation]?.[phraseKey] as Phrase<TKey>;
if (translatedPhrase) {
return typeof translatedPhrase === 'function' ? translatedPhrase(...phraseParameters) : translatedPhrase;
const fallbacktranslatedPhrase = getTranslatedPhrase(fallbackLanguage, phraseKey, null, ...phraseParameters);

if (fallbacktranslatedPhrase) {
return fallbacktranslatedPhrase;
}

if (languageAbbreviation !== CONST.LOCALES.DEFAULT) {
Log.alert(`${phraseKey} was not found in the ${languageAbbreviation} locale`);
if (fallbackLanguage !== CONST.LOCALES.DEFAULT) {
Log.alert(`${phraseKey} was not found in the ${fallbackLanguage} locale`);
}

// Phrase is not translated, search it in default language (en)
translatedPhrase = translations?.[CONST.LOCALES.DEFAULT]?.[phraseKey] as Phrase<TKey>;
return getTranslatedPhrase(CONST.LOCALES.DEFAULT, phraseKey, null, ...phraseParameters);
}

/**
* Return translated string for given locale and phrase
*
* @param [desiredLanguage] eg 'en', 'es-ES'
* @param [phraseParameters] Parameters to supply if the phrase is a template literal.
*/
function translate<TKey extends TranslationPaths>(desiredLanguage: 'en' | 'es' | 'es-ES' | 'es_ES', phraseKey: TKey, ...phraseParameters: PhraseParameters<Phrase<TKey>>): string {
// Search phrase in full locale e.g. es-ES
const language = desiredLanguage === CONST.LOCALES.ES_ES_ONFIDO ? CONST.LOCALES.ES_ES : desiredLanguage;
// Phrase is not found in full locale, search it in fallback language e.g. es
const languageAbbreviation = desiredLanguage.substring(0, 2) as 'en' | 'es';

const translatedPhrase = getTranslatedPhrase(language, phraseKey, languageAbbreviation, ...phraseParameters);
if (translatedPhrase) {
return typeof translatedPhrase === 'function' ? translatedPhrase(...phraseParameters) : translatedPhrase;
return translatedPhrase;
}

// Phrase is not found in default language, on production and staging log an alert to server
Expand Down
15 changes: 11 additions & 4 deletions src/libs/PersonalDetailsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,13 @@ Onyx.connect({
});

function getDisplayNameOrDefault(passedPersonalDetails?: Partial<PersonalDetails> | null, defaultValue = '', shouldFallbackToHidden = true, shouldAddCurrentUserPostfix = false): string {
let displayName = passedPersonalDetails?.displayName ? passedPersonalDetails.displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '') : '';
let displayName = passedPersonalDetails?.displayName ?? '';

// If the displayName starts with the merged account prefix, remove it.
if (displayName.startsWith(CONST.MERGED_ACCOUNT_PREFIX)) {
hurali97 marked this conversation as resolved.
Show resolved Hide resolved
// Remove the merged account prefix from the displayName.
displayName = displayName.substring(CONST.MERGED_ACCOUNT_PREFIX.length);
}

// If the displayName is not set by the user, the backend sets the diplayName same as the login so
// we need to remove the sms domain from the displayName if it is an sms login.
Expand All @@ -37,9 +43,10 @@ function getDisplayNameOrDefault(passedPersonalDetails?: Partial<PersonalDetails
displayName = `${displayName} (${Localize.translateLocal('common.you').toLowerCase()})`;
}

const fallbackValue = shouldFallbackToHidden ? Localize.translateLocal('common.hidden') : '';

return displayName || defaultValue || fallbackValue;
if (displayName) {
return displayName;
}
return defaultValue || (shouldFallbackToHidden ? Localize.translateLocal('common.hidden') : '');
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ function isAnnounceRoom(report: OnyxEntry<Report>): boolean {
* Whether the provided report is a default room
*/
function isDefaultRoom(report: OnyxEntry<Report>): boolean {
return [CONST.REPORT.CHAT_TYPE.POLICY_ADMINS, CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE, CONST.REPORT.CHAT_TYPE.DOMAIN_ALL].some((type) => type === getChatType(report));
return CONST.DEFAULT_POLICY_ROOM_CHAT_TYPES.some((type) => type === getChatType(report));
}

/**
Expand Down
Loading