search

jwplayer / ott-web-app

Reference implementation for JWP-powered apps
Apache License 2.0
69 stars 52 forks source link

Feat / improve generate translations script #482

Closed ChristiaanScheermeijer closed 3 weeks ago

ChristiaanScheermeijer commented 3 months ago

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.

github-actions[bot] commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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.

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

@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!

ChristiaanScheermeijer commented 3 weeks ago

@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).