chore: precompile translations to AST#1929
Conversation
achou11
left a comment
There was a problem hiding this comment.
Functionality seems to work as intended, but think there's room to clean up the implementation.
I verified that dynamic imports are working by checking using expo atlas (see https://docs.expo.dev/guides/analyzing-bundles/).
I was trying to test changes to the system settings using an emulator but seems like useLocales() doesn't update when that occurs. Might be a dev-specific issue, as it doesn't work on dev branch either (but works with built APK). Would be good to build an apk and test that changing the language via phone settings has expected results in the app.
There was a problem hiding this comment.
Mind explaining why the changes here are necessary? It's not immediately clear to me... (aside from tests failing without the change)
There was a problem hiding this comment.
jest doesn't support esm dynamic importing out of the box. So im just resolving the promise before hand in the jest environment (so it is no longer a dynamic import). We also dont need any other languages besides english in our jest tests, so i was only resolving english. But on reflection i'm just going to resolve all the languages in case we need them for future tests
| import {StyleSheet, Text} from 'react-native'; | ||
| import {useLocales} from 'expo-localization'; | ||
| import {useQuery, keepPreviousData} from '@tanstack/react-query'; | ||
| import type {MessageFormatElement} from '@formatjs/icu-messageformat-parser'; |
There was a problem hiding this comment.
Probably preferable to use the export from react-intl instead:
| import type {MessageFormatElement} from '@formatjs/icu-messageformat-parser'; | |
| import type {MessageFormatElement} from 'react-intl'; |
There was a problem hiding this comment.
I would suggest updating this script to use the official formatjs tooling for compilation.
Implemented an example alternative script that maintains existing behavior but uses @formatjs/cli-lib: 86219bc
| import path from 'node:path'; | ||
| import fs from 'node:fs'; | ||
| import {readFile, writeFile} from 'node:fs/promises'; | ||
| import {parse} from '@formatjs/icu-messageformat-parser'; |
There was a problem hiding this comment.
see comment about updating this file as a whole. seeing usage of this module - which is pretty low-level and probably not meant for direct usage - raised some alarms for me.
| const {data: messagesToUse, isPending} = useQuery({ | ||
| queryKey: ['messages', ...languages], | ||
| queryFn: async () => { | ||
| const results = await Promise.all( | ||
| // reversing languages mean the highest priority languages get merged last and overwites the lower priority languages | ||
| languages | ||
| .reverse() | ||
| .map( | ||
| tag => | ||
| localeImports[tag]?.().then(m => m.default) ?? | ||
| Promise.resolve({}), | ||
| ), | ||
| ); | ||
| const merged: Record<string, MessageFormatElement[]> = {}; | ||
| for (const msgs of results) { | ||
| Object.assign(merged, msgs); | ||
| } | ||
| return merged; | ||
| }, | ||
| staleTime: Infinity, | ||
| // see: https://tanstack.com/query/latest/docs/react/guides/paginated-queries#better-paginated-queries-with-placeholderdata | ||
| placeholderData: keepPreviousData, | ||
| }); |
There was a problem hiding this comment.
While this works, I think it'd be more idiomatic to use useQueries here. The main reasoning is to isolate each language with their own query to avoid unnecessary work when they change.
Implemented an example here: d7b1135 (note that it might need some adjustments...)
| import languages from '../src/frontend/languages.json' with {type: 'json'}; | ||
| import messages from '../translations/messages.json' with {type: 'json'}; | ||
|
|
||
| const TRANSLATIONS_DIR = new URL('../translations/', import.meta.url).pathname; |
There was a problem hiding this comment.
Need to update this in order to be used on a non-posix OS.
| const TRANSLATIONS_DIR = new URL('../translations/', import.meta.url).pathname; | |
| const TRANSLATIONS_DIR = fileURLToPath(new URL('../translations/', import.meta.url)); |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
achou11
left a comment
There was a problem hiding this comment.
Couple of non-blocking suggestions but otherwise changes look good
| if ( | ||
| !Object.prototype.hasOwnProperty.call( | ||
| LANGUAGE_NAME_TRANSLATIONS, | ||
| languageCode, | ||
| ) |
There was a problem hiding this comment.
Very subjective: this feels more readable at a glance and achieves the same thing:
| if ( | |
| !Object.prototype.hasOwnProperty.call( | |
| LANGUAGE_NAME_TRANSLATIONS, | |
| languageCode, | |
| ) | |
| if (!(languageCode in LANGUAGE_NAME_TRANSLATIONS)) { |
| * | ||
| * @description The tanstack query option `placeholderData` is set to `keepPreviousData`. This means this query will only ever be `pending` on initial load | ||
| */ | ||
| export function useLanguageQueries(languageCodes: AvailableLanguageTag[]) { |
There was a problem hiding this comment.
defining a separate hook that just wraps a query hook seems unnecessary right now. especially since this only used in one place, I would recommend inlining it where it's used.
If you have a need for reusing it elsewhere, usually the recommended approach is to define a query options object and passing that into the relevant query hook in the calling component (see https://tkdodo.eu/blog/the-query-options-api).
closes #1879
closes #539
Also Lazy load translations so they are not all initially loaded in memory. Uses useQuery to deal with the async nature of dynamically loading the translations.
I also had to update the tsconfig to
"module": "esnext"as it was throwing an error with the dynamic imports