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

chore: update translations compilation script and use AST output #582

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

achou11
Copy link
Member

@achou11 achou11 commented Aug 5, 2024

Closes #539

Updates the build-translations script in the following ways:

  • rely more on @formatjs/cli and @format/cli-lib to turn extracted messages into app-ready translations

  • output an AST instead of the normal message format. reasons for this are:

    1. it allows react-intl to skip work parsing the data at runtime, improving performance (https://formatjs.io/docs/guides/advanced-usage#pre-compiling-messages)

    2. it allows us to avoid including the relevant parser in the bundled app, which reduces bundle size (https://formatjs.io/docs/guides/advanced-usage#react-intl-without-parser-40-smaller) EDIT: had to punt on this because if you're missing message keys for a language, it will fail to parse the code-defined defaultMessage (which is not in AST form) verbatim and thus not actually interpolate placeholders values, which is not desired. Removed in a subsequent commit but would be nice to take advantage of this eventually.

    the downside of using the AST is that it's larger in size compared to regular string messages, although it can be efficiently compressed. This means that given the current approach of loading the translations into the app (i.e. all at once, not lazily), this will probably result in slightly more memory being used.

    I'm open to removing the change to using the AST, in which case the issue referenced would not be closed and this PR would solely be making our custom script rely more on @formatjs tooling.

  • less boilerplate code to make it more linear in terms of reading (at the small cost of functional purity/modularity)

@achou11 achou11 requested a review from ErikSin August 5, 2024 20:08
@achou11
Copy link
Member Author

achou11 commented Aug 5, 2024

Something i'm unsure about is this note in the docs:

Since this approach uses AST as the data source, changes to @formatjs/icu-messageformat-parser's AST will require cache invalidation.

not sure if that means we need some code to handle this or if that's just done internally and they're just letting us know 🤔

EDIT: I don't think this affects us because we generate the AST at build time and it's never persisted beyond the lifetime of the app. This would be more of a problem if we were storing the AST and loading it into memory in the case that the app uses some updated AST format introduced by updating the library

@achou11 achou11 force-pushed the ac/build-translations-improvements branch 2 times, most recently from 3015fad to 4bb7eb1 Compare August 13, 2024 14:40
@achou11 achou11 force-pushed the ac/build-translations-improvements branch 5 times, most recently from e06b44e to a8bb61d Compare August 21, 2024 16:23
@achou11 achou11 force-pushed the ac/build-translations-improvements branch 6 times, most recently from 0c90894 to 7c3def7 Compare September 9, 2024 20:54
@achou11 achou11 force-pushed the ac/build-translations-improvements branch 2 times, most recently from 1c82f04 to 85f5040 Compare September 17, 2024 14:37
@achou11 achou11 force-pushed the ac/build-translations-improvements branch from 85f5040 to 6e2e5d0 Compare September 25, 2024 15:26
@achou11 achou11 force-pushed the ac/build-translations-improvements branch from 6e2e5d0 to cc7b2d2 Compare October 9, 2024 15:04
@achou11
Copy link
Member Author

achou11 commented Oct 9, 2024

CI error due to incorrectly formatted translation that made it into develop EDIT: resolved after latest rebase on develop

"message": "Las fotos se van a sincronizar en un tamaño reducido. Tu dispositivo <bold> no <bold> sincornizará audio y video."

@achou11 achou11 force-pushed the ac/build-translations-improvements branch from cc7b2d2 to 6ee6418 Compare October 31, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potentially address warning from @formatjs/intl about not pre-compiling messages
1 participant