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.

Sign up for GitHub

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: allow Truthy values to be returned correctly from translation #38667

Closed
wants to merge 2 commits into from
Closed

fix: allow Truthy values to be returned correctly from translation #38667

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eabe626
fix: allow truthy values to be returned correctly
hurali97 Mar 20, 2024
1731d9f
refactor: update getTranslatedPhrase to return a value and a validity…
hurali97 Mar 21, 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
2 changes: 1 addition & 1 deletion src/libs/Localize/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ function translate<TKey extends TranslationPaths>(desiredLanguage: 'en' | 'es' |
const languageAbbreviation = desiredLanguage.substring(0, 2) as 'en' | 'es';

const translatedPhrase = getTranslatedPhrase(language, phraseKey, languageAbbreviation, ...phraseParameters);
if (translatedPhrase) {
if (translatedPhrase !== null && translatedPhrase !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@hurali97 I believe the correct way to fix this bug is to return the result of the translation as it is, similar to what was previously done: Here is a simple code diff, please give me your thoughts:

code 
diff --git a/src/libs/Localize/index.ts b/src/libs/Localize/index.ts
index 39a8741990..c54f07dacb 100644
--- a/src/libs/Localize/index.ts
+++ b/src/libs/Localize/index.ts
@@ -95,7 +95,7 @@ function getTranslatedPhrase<TKey extends TranslationPaths>(
     phraseKey: TKey,
     fallbackLanguage: 'en' | 'es' | null = null,
     ...phraseParameters: PhraseParameters<Phrase<TKey>>
-): string | null {
+): string {
     // Get the cache for the above locale
     const cacheForLocale = translationCache.get(language);
 
@@ -121,7 +121,17 @@ function getTranslatedPhrase<TKey extends TranslationPaths>(
     }
 
     if (!fallbackLanguage) {
-        return null;
+        // Phrase is not found in default language, on production and staging log an alert to server
+        // on development throw an error
+        if (Config.IS_IN_PRODUCTION || Config.IS_IN_STAGING) {
+            const phraseString: string = Array.isArray(phraseKey) ? phraseKey.join('.') : phraseKey;
+            Log.alert(`${phraseString} was not found in the en locale`);
+            if (userEmail.includes(CONST.EMAIL.EXPENSIFY_EMAIL_DOMAIN)) {
+                return CONST.MISSING_TRANSLATION;
+            }
+            return phraseString;
+        }
+        throw new Error(`${phraseKey} was not found in the default language`);
     }
 
     // Phrase is not found in full locale, search it in fallback language e.g. es
@@ -151,22 +161,7 @@ function translate<TKey extends TranslationPaths>(desiredLanguage: 'en' | 'es' |
     // 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 !== null && translatedPhrase !== undefined) {
-        return translatedPhrase;
-    }
-
-    // Phrase is not found in default language, on production and staging log an alert to server
-    // on development throw an error
-    if (Config.IS_IN_PRODUCTION || Config.IS_IN_STAGING) {
-        const phraseString: string = Array.isArray(phraseKey) ? phraseKey.join('.') : phraseKey;
-        Log.alert(`${phraseString} was not found in the en locale`);
-        if (userEmail.includes(CONST.EMAIL.EXPENSIFY_EMAIL_DOMAIN)) {
-            return CONST.MISSING_TRANSLATION;
-        }
-        return phraseString;
-    }
-    throw new Error(`${phraseKey} was not found in the default language`);
+    return getTranslatedPhrase(language, phraseKey, languageAbbreviation, ...phraseParameters);
 }
 
 /**

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think we should refactor the code in getTranslatedPhrase , Previously we were checking the existence of a key inside the Translations object, but now we check the value of the translation which doesn't look correct IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @hurali97 Here is my latest attempt to address the concerns raised in the previous comment: I believe we should refactor the getTranslatedPhrase code and introduce a new flag isValidKey in its response. This way, the function should return both the value and the validity of the key. Here is the updated code:

Code 

diff --git a/src/libs/Localize/index.ts b/src/libs/Localize/index.ts
index 39a8741990..993561a0e6 100644
--- a/src/libs/Localize/index.ts
+++ b/src/libs/Localize/index.ts
@@ -72,6 +72,12 @@ const translationCache = new Map<ValueOf<typeof CONST.LOCALES>, Map<TranslationP
     }, [] as Array<[ValueOf<typeof CONST.LOCALES>, Map<TranslationPaths, string>]>),
 );
 
+
+type CacheValue = {
+    value: string;
+    isValidKey: boolean
+}
+
 /**
  * Helper function to get the translated string for given
  * locale and phrase. This function is used to avoid
@@ -95,7 +101,7 @@ function getTranslatedPhrase<TKey extends TranslationPaths>(
     phraseKey: TKey,
     fallbackLanguage: 'en' | 'es' | null = null,
     ...phraseParameters: PhraseParameters<Phrase<TKey>>
-): string | null {
+): CacheValue {
     // Get the cache for the above locale
     const cacheForLocale = translationCache.get(language);
 
@@ -105,30 +111,30 @@ function getTranslatedPhrase<TKey extends TranslationPaths>(
 
     // If the phrase is already translated, return the translated value
     if (valueFromCache) {
-        return valueFromCache;
+        return {value: valueFromCache, isValidKey: true}
     }
 
     const translatedPhrase = translations?.[language]?.[phraseKey] as Phrase<TKey>;
 
     if (translatedPhrase) {
         if (typeof translatedPhrase === 'function') {
-            return translatedPhrase(...phraseParameters);
+            return {value: translatedPhrase(...phraseParameters), isValidKey: true};
         }
 
         // We set the translated value in the cache only for the phrases without parameters.
         cacheForLocale?.set(phraseKey, translatedPhrase);
-        return translatedPhrase;
+        return {value: translatedPhrase, isValidKey: true};
     }
 
     if (!fallbackLanguage) {
-        return null;
+        return {value: '', isValidKey: false}
     }
 
     // Phrase is not found in full locale, search it in fallback language e.g. es
     const fallbacktranslatedPhrase = getTranslatedPhrase(fallbackLanguage, phraseKey, null, ...phraseParameters);
 
-    if (fallbacktranslatedPhrase) {
-        return fallbacktranslatedPhrase;
+    if (fallbacktranslatedPhrase.isValidKey) {
+        return fallbacktranslatedPhrase
     }
 
     if (fallbackLanguage !== CONST.LOCALES.DEFAULT) {
@@ -151,9 +157,9 @@ function translate<TKey extends TranslationPaths>(desiredLanguage: 'en' | 'es' |
     // 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 !== null && translatedPhrase !== undefined) {
-        return translatedPhrase;
+    const {value, isValidKey} = getTranslatedPhrase(language, phraseKey, languageAbbreviation, ...phraseParameters);
+    if (isValidKey) {
+        return value;
     }
 
     // Phrase is not found in default language, on production and staging log an alert to server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fedirjh Your latest code looks nice and I have incorporated it with just renaming the type name from CacheValue to TranslatedValue. Is that fine with you?

I believe the correct way to fix this bug is to return the result of the translation as it is, similar to what was previously done

If we did that we will have the same bug where we show the translation key in UI for the value which is an empty string. To fill you in, previously we were checking for Truthiness of translatedPhrase and if it's an empty string, which can happen because of variables, we will have an empty string as translatedPhrase, which is what is expected. So in such cases, returning phraseKey instead would be a violation.

But I guess it's also fixed by your latest code, so that's fine 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

So in such cases, returning phraseKey instead would be a violation.

Yes, we are in agreement. By "the result of the translation as it is" I meant the value, even if it's empty or null, not the key.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think that the code was simpler and just as clear before - strings (including empty strings) are valid results, but null or undefined are not.

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e: if the translation isn't found, getTranslatedPhrase should just return null, rather than {value: '', isValidKey: false}. If the translation is found and happens to be an empty string, that's fine

Copy link
Contributor

Choose a reason for hiding this comment

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

are valid results, but null or undefined are not.

@roryabraham Yes, but this won't tell us if the key exists or not. with previous code , null or undefined means that it's either :

  1. the key doesn't exit.
  2. the key exists, but its value is null or undefined. this can happen if we have a function maybe?

In the second case, In staging or production it will log an alert to the server about a key not found, which is not what we want?

return translatedPhrase;
}

Expand Down
Loading