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

Feat / improve generate translations script #482

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

ChristiaanScheermeijer
Copy link
Collaborator

Description

This PR updates the i18next script that extracts all translations from the source files. I've added that this happens for each platform in the platforms folder. The workspace dependencies are resolved from the package.json and used in the i18next script.

The settings can be changed by a simple .i18n.json file in the platform directory (not used in the public repo). For instance, the bundle option creates TypeScript files, exporting all JSON files to be bundled with the platform code.

I also added a pass that checks and fills missing translations between the platforms. This is a QOL feature when syncing new changes from the public repo when translations are added.

Copy link

github-actions bot commented Apr 3, 2024

Visit the preview URL for this PR (updated for commit df4efb3):

https://ottwebapp--pr482-feat-platform-transl-kzlwaxa4.web.app

(expires Fri, 12 Jul 2024 08:40:10 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

@dbudzins
Copy link
Contributor

dbudzins commented Apr 8, 2024

@ChristiaanScheermeijer I got a little way though this PR, and I started to question the approach a bit. Some of my concerns:

  1. Hybrid approach where translation tags are baked into packages but translation values are in platforms
  2. Complicates translation reuse since values and namespaces may span multiple packages, but can't be shared
  3. Unsure whether other platforms will be able to use i18next. I assume react native can, so hooks and ui-react are fine, but usage in common may not be
  4. Current solution relies on several assumptions / conventions, such as only parsing @jwp packages and /src directory for translation keys

Have you considered alternatives such as each packages maintaining its own translations and/or having platforms pass translations into dependent code?

@ChristiaanScheermeijer
Copy link
Collaborator Author

Thanks for reviewing and raising this @dbudzins

I agree that the translations are a bit weird in the current setup. We tried to make it as easy as possible because managing translations seems one of the hardest things to do right 😅. Perhaps we can use this PR to think and discuss better alternatives.

I like the idea of moving the translation parser responsibility to each platform. This removes any logic from the root and each platform gains full control.

The only part missing is the automatic syncing between platforms. This already caught us a few times after syncing the open-source repo with translation changes.

I see the following options:

Install and configure i18next-parser per platform

This is your suggestion and seems valid to me. The parser needs to traverse to the packages folder, but we also do this for TypeScript and Vite.

  • Remove i18next-parser from root package.json
  • Install i18next-parser in platforms/web
  • Move the ./scripts/i18next folder to ./platforms/web/scripts/i18next
    • Although this script is reusable, we might make it configure and accept parameters instead
  • Move i18next-parser.config.js to platforms/web
  • Move i18next scripts from root package.json to platforms/web/package.json
  • Update documentation

Platforms & packages translations

We can take the above one step further by making each package responsible for their translations as well. Each package will have an i18n folder and scripts to generate the translations. Then, we can add an i18next script in the root to run each workspace i18next script separately.

Platforms can consume the translations by copying or bundling them with full control. Customising translations may be challenging because we want to change specific translations used in the packages.

I'm not sure yet how to manage translations when passing them to the packages. Will this not make it a lot harder to keep track of which translations are added/missing?

Unless we leverage TS for this, I'm afraid this can become quite cumbersome and difficult to keep in sync.

Let me know what you think or have more options to explore 😄

@dbudzins
Copy link
Contributor

dbudzins commented Apr 11, 2024

@ChristiaanScheermeijer I would lean towards the per-platform approach over the per-package approach for the reasons you've identified. It seems safer to me to rely on crawling imports to find all the potential translations instead of crawling the workspaces manually (also will tree shake the unused translations per platform.)

It might be handy to make a package to package sync script that can just be run from a standalone yarn command.

Is it ok to have i18next.t baked into common or should we try to avoid this so that common can be used everywhere even non-react platforms?

@ChristiaanScheermeijer
Copy link
Collaborator Author

Thanks @dbudzins

I would lean towards the per-platform approach over the per-package approach for the reasons you've identified. It seems safer to me to rely on crawling imports to find all the potential translations instead of crawling the workspaces manually (also will tree shake the unused translations per platform.)

Agree, I will look into that!

It might be handy to make a package to package sync script that can just be run from a standalone yarn command.

Do you think this can live in the workspace root, or should we also make this platform-specific?

Is it ok to have i18next.t baked into common or should we try to avoid this so that common can be used everywhere even non-react platforms?

Luckily, i18next and react-i18next are separate packages. The i18next package is framework- and even platform-agnostic, so we are safe to use that in the common package. I'm not closed to alternatives or not using i18next (it is quite a performance killer), but replacing it will not be an easy task 😅

@ChristiaanScheermeijer
Copy link
Collaborator Author

ChristiaanScheermeijer commented Apr 11, 2024

@dbudzins I gave it a shot, let me know what you think. I will update the documentation if we want to proceed with this change.

Usage:

# parses all translations based on the installed workspace dependencies (with a `./src` folder)
$ cd platforms/web
$ yarn web i18next
# fills missing translations in the target platform (using the source platform as base)
# E.g. web --> app when translation in app is empty
$ yarn i18next:sync
Usage:
  yarn i18next:sync <sourcePlatform> <targetPlatform>

Use this script to fill missing translations in the target platform
using the translations of the source platform as base.

Example:
  yarn i18next:sync web app

Missing arguments!

Let me know what you think!

Copy link
Contributor

@dbudzins dbudzins left a comment

Choose a reason for hiding this comment

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

I think this is an ok way to start, pending some robustness improvements discussed below. Upon further digging, it seems like we might just be able to build 1 large global list of translations for the whole workspace and then, if we can figure out how (I haven't yet) tree shake the used translations at build time. That'd be a really slick solution.

platforms/web/package.json Outdated Show resolved Hide resolved
platforms/web/scripts/i18next/parse-translations.ts Outdated Show resolved Hide resolved
platforms/web/scripts/i18next/parse-translations.ts Outdated Show resolved Hide resolved
platforms/web/scripts/i18next/parse-translations.ts Outdated Show resolved Hide resolved
platforms/web/scripts/i18next/parse-translations.ts Outdated Show resolved Hide resolved
platforms/web/scripts/i18next/parse-translations.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dbudzins dbudzins left a comment

Choose a reason for hiding this comment

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

lgtm

@ChristiaanScheermeijer ChristiaanScheermeijer added JWP Audit This PR needs a review from an JWP employee Needs discussion This PR needs to be discussed before merging labels Jun 4, 2024
@ChristiaanScheermeijer
Copy link
Collaborator Author

@dbudzins @AntonLantukh I think this PR is ready to be merged. I rebased the branch with develop and validated it by running yarn workspace @jwp/ott-web run i18next, which removed all profile-related translations (as expected since this feature was removed).

@ChristiaanScheermeijer ChristiaanScheermeijer merged commit 2fa5bfd into develop Jun 12, 2024
10 checks passed
@ChristiaanScheermeijer ChristiaanScheermeijer deleted the feat/platform-translations branch June 12, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JWP Audit This PR needs a review from an JWP employee Needs discussion This PR needs to be discussed before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants