[WEB-3153] improvement: add support for nested translations and ICU formatting#6411
Conversation
|
Pull Request Linked with Plane Issues Comment Automatically Generated by Plane |
1 similar comment
|
Pull Request Linked with Plane Issues Comment Automatically Generated by Plane |
WalkthroughThe pull request introduces significant changes to the internationalization (i18n) package, including the addition of a new dependency, Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/i18n/src/types/translation.ts (2)
1-3: Consider adding JSDoc documentation and ICU message format validation.The interface correctly enables nested translations with its recursive design. However, consider these improvements:
- Add JSDoc documentation explaining the purpose and usage
- Consider adding type validation for ICU message format strings
Example enhancement:
+/** + * Represents a translation object that can contain either direct string translations + * or nested translation objects. + * @example + * { + * "welcome": "Hello {name}!", + * "notifications": { + * "count": "You have {count, plural, one{# notification} other{# notifications}}" + * } + * } + */ export interface ITranslation { + // ICU MessageFormat strings or nested translation objects [key: string]: string | ITranslation; }
5-7: Consider adding type safety for locale strings.The interface provides a clean structure for organizing translations by locale. Consider enhancing type safety by:
- Using a union type of supported locales instead of string
- Adding JSDoc documentation listing supported locales
Example enhancement:
+/** Supported locale codes */ +export type SupportedLocale = 'en' | 'es' | 'fr' | 'de' | 'ja' | 'ko' | 'zh'; + +/** + * Maps locale codes to their respective translations. + * @see SupportedLocale for list of supported locales + */ export interface ITranslations { - [locale: string]: ITranslation; + [locale in SupportedLocale]: ITranslation; }packages/i18n/src/store/index.ts (1)
34-49: Optimize translation loading to improve performanceCurrently, the
loadTranslationsmethod loads all translation files for all supported languages upon initialization. This could impact performance, especially if the translation files are large. Consider loading translations on demand when a language is set or when it's actually needed.Apply this diff to refactor the translation loading:
- private async loadTranslations() { + private async loadTranslations(locale: TLanguage) { try { - // dynamic import of translations - const translations = { - en: (await import("../locales/en/translations.json")).default, - fr: (await import("../locales/fr/translations.json")).default, - es: (await import("../locales/es/translations.json")).default, - ja: (await import("../locales/ja/translations.json")).default, - "zh-CN": (await import("../locales/zh-CN/translations.json")).default, - }; - this.translations = translations; + const translation = (await import(`../locales/${locale}/translations.json`)).default; + this.translations[locale] = translation; this.messageCache.clear(); // Clear cache when translations change } catch (error) { console.error("Failed to load translations:", error); } }Update the constructor and
setLanguagemethod to load translations for the current locale:constructor() { makeAutoObservable(this); this.initializeLanguage(); - this.loadTranslations(); + this.loadTranslations(this.currentLocale); } setLanguage(lng: TLanguage): void { try { if (!this.isValidLanguage(lng)) { throw new Error(`Invalid language: ${lng}`); } localStorage.setItem(STORAGE_KEY, lng); this.currentLocale = lng; this.messageCache.clear(); // Clear cache when language changes document.documentElement.lang = lng; + this.loadTranslations(lng); } catch (error) { console.error("Failed to set language:", error); } }packages/i18n/src/context/index.tsx (1)
6-6: Consider adding a custom error message for the null context case.While the null default value is appropriate, adding a descriptive error message in the default value could help developers identify missing providers more easily.
-export const TranslationContext = createContext<TranslationStore | null>(null); +export const TranslationContext = createContext<TranslationStore | null>((() => { + throw new Error('TranslationContext: using context without provider'); +})());packages/i18n/src/hooks/use-translation.ts (2)
14-22: Consider improving JSDoc accuracy.The @returns tag is repeated multiple times. Consider using @Property tags for describing the returned object's properties.
/** * Provides the translation store to the application - * @returns {TTranslationStore} - * @returns {(key: string, params?: Record<string, any>) => string} t: method to translate the key with params - * @returns {TLanguage} currentLocale - current locale language - * @returns {(lng: TLanguage) => void} changeLanguage - method to change the language - * @returns {ILanguageOption[]} languages - available languages + * @returns {TTranslationStore} The translation store object + * @property {Function} t - Method to translate the key with params + * @property {TLanguage} currentLocale - Current locale language + * @property {Function} changeLanguage - Method to change the language + * @property {ILanguageOption[]} languages - Available languages * @throws {Error} if the TranslationProvider is not used */
29-34: Consider optimizing the store method binding.The
bindcall in the return object will create a new function on every render. Consider moving it to auseMemoor the initialization phase.export function useTranslation(): TTranslationStore { const store = useContext(TranslationContext); if (!store) { throw new Error('useTranslation must be used within a TranslationProvider'); } + const translationFn = React.useMemo(() => store.t.bind(store), [store]); + return { - t: store.t.bind(store), + t: translationFn, currentLocale: store.currentLocale, changeLanguage: (lng: TLanguage) => store.setLanguage(lng), languages: store.availableLanguages, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
packages/i18n/package.json(1 hunks)packages/i18n/src/components/index.tsx(0 hunks)packages/i18n/src/components/store.ts(0 hunks)packages/i18n/src/config/index.ts(0 hunks)packages/i18n/src/constants/index.ts(1 hunks)packages/i18n/src/constants/language.ts(1 hunks)packages/i18n/src/context/index.tsx(1 hunks)packages/i18n/src/hooks/use-translation.ts(1 hunks)packages/i18n/src/index.ts(1 hunks)packages/i18n/src/store/index.ts(1 hunks)packages/i18n/src/types/index.ts(1 hunks)packages/i18n/src/types/language.ts(1 hunks)packages/i18n/src/types/translation.ts(1 hunks)web/core/lib/wrappers/store-wrapper.tsx(2 hunks)
💤 Files with no reviewable changes (3)
- packages/i18n/src/components/store.ts
- packages/i18n/src/components/index.tsx
- packages/i18n/src/config/index.ts
✅ Files skipped from review due to trivial changes (3)
- packages/i18n/src/constants/index.ts
- packages/i18n/src/types/index.ts
- packages/i18n/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-web
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (8)
packages/i18n/src/types/translation.ts (1)
1-7: Well-designed foundation for the i18n system!The type definitions provide a solid foundation for:
- Nested translations
- ICU MessageFormat support
- Locale-based organization
packages/i18n/src/store/index.ts (1)
14-165: Overall implementation is clean and follows best practicesThe
TranslationStoreclass is well-structured, leveraging MobX effectively to manage state. The methods provided offer a robust solution for handling translations and language settings.packages/i18n/src/types/language.ts (1)
1-6: Type definitions are clear and enhance type safetyThe
TLanguagetype andILanguageOptioninterface are properly defined, improving type safety and consistency across the application.packages/i18n/src/constants/language.ts (1)
1-13: Language constants are well-defined and consistentThe
FALLBACK_LANGUAGE,SUPPORTED_LANGUAGES, andSTORAGE_KEYconstants are appropriately set up, enhancing the maintainability and clarity of the language configuration.packages/i18n/src/context/index.tsx (1)
15-19: LGTM! Clean implementation of the TranslationProvider.The implementation follows React best practices:
- Uses observer pattern for reactivity
- Initializes store only once using useState callback
- Properly types the props interface
packages/i18n/src/hooks/use-translation.ts (1)
7-12: LGTM! Well-defined type interface for the translation store.The TTranslationStore type clearly defines all required methods and properties for the translation functionality.
web/core/lib/wrappers/store-wrapper.tsx (1)
56-56: LGTM! Proper type handling for language change.The type casting ensures type safety while maintaining the existing functionality.
packages/i18n/package.json (1)
13-14: LGTM! Appropriate dependency for ICU message formatting.The addition of
intl-messageformataligns with the PR's objective to support ICU MessageFormat for translations.Let's verify if this is the latest stable version:
✅ Verification successful
Version ^10.7.11 of intl-messageformat is the latest stable release
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest version of intl-messageformat npm view intl-messageformat versions --json | jq -r '.[-1]'Length of output: 69
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/i18n/src/store/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
packages/i18n/src/store/index.ts (1)
71-74: Browser language detection may not handle region codes correctlyIn the
getBrowserLanguage()method, splittingnavigator.languageby'-'and taking the first part may cause issues for languages with region codes like'zh-CN'or'pt-BR'. This could result in'zh'or'pt', which may not match your supported languages, leading to a fallback to the default language instead of the user's preferred language.Apply this diff to fix the issue:
private getBrowserLanguage(): TLanguage { + if (typeof navigator === "undefined") { + return FALLBACK_LANGUAGE; + } - const browserLang = navigator.language.split("-")[0]; + const browserLang = navigator.language; + if (this.isValidLanguage(browserLang)) { + return browserLang; + } + const baseLang = browserLang.split("-")[0]; + return this.isValidLanguage(baseLang) ? (baseLang as TLanguage) : FALLBACK_LANGUAGE; }
2 participants
Description
Translation System Enhancement
Changes
Example Usage
Type of Change
References
WEB-3153
Summary by CodeRabbit
New Features
TranslationProvidercomponent for managing translations.Dependency Updates
intl-messageformatlibrary for improved translation handling.Refactor