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

Refactoring the web translations #1777

Merged
merged 16 commits into from
Nov 28, 2024
Merged

Refactoring the web translations #1777

merged 16 commits into from
Nov 28, 2024

Conversation

lslezak
Copy link
Contributor

@lslezak lslezak commented Nov 21, 2024

Problem

  • The web translations still use the original Cockpit mechanism for creating and loading the translations
  • That uses a server based logic which has quite some drawbacks
    • We have to reimplement the very same logic in a Webpack plugin to make it work also in the local development server (npm run server). And unfortunately we cannot share the code because it is Rust vs. Javascript...
    • This will not work in the Agama online demo site with completely static files (without any server side logic, everything needs to be on the client side).
    • For me the original logic was quite strange, why the server should decide which translations should the client use? It should be completely handled on the client side, there is no reason for the special server logic. The server should just report the available translations and let the client to pick and use the right one.
    • The old implementation has also some ugly parts like requiring a global Javascript variable...

Solution

  • Move the translation logic from the server to the client (browser)
  • Use dynamic imports instead of HTTP redirection

Implementation details

  • Removed the po.js handler from the Agama HTTP server
  • Removed the po.js handler from the Webpack development server
  • Removed the original Gettext PO files from Git, replaced by already converted JS files
  • Removed the Cockpit Webpack plugin for converting PO files to JS, replaced by a simple script with similar functionality. (As a bonus the code was fixed to avoid adding unnecessary null values into the output.)
  • Added dynamic import for downloading the requested translations
  • Some code cleanup (removing the global JS object)
  • Moved the jed and gettext-parser NPM dependencies to the PO->JS converter, that means those NPM packages are installed only when converting the PO files to JS (basically only in the GitHub action which merges them), they are not installed when building the Agama web frontend

Testing

  • Tested manually, loading the translations works fine
  • I manually triggered the GitHub Action which merges the translations from Weblate. It works fine, the test run opened this pull request with updated translations (there are more changes as it was executed in the same branch as this pull request).

TODO

  • Improve the mechanism for finding the appropriate translation file, the current simple logic only uses the language name and cannot load the pt_BR translations which additionally include the country code
  • Rewrite the Webpack plugin for converting the *.po file to *.js into a standalone script (Webpack needs to load the JS files during compilation, the dynamically generated JS files are not found)
  • Update the import from Weblate, run the new script and convert the *.po files to *.js during import (the Git repository will contain the already converted *.js translations instead of the original *.po Gettext files)
  • Fix/improve the unit tests

@lslezak lslezak marked this pull request as ready for review November 26, 2024 09:46
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Really nice improvement. Just a minor comment. Otherwise, LGTM.

@@ -230,6 +230,30 @@ function InstallerL10nProvider({ children }: { children?: React.ReactNode }) {
reload(newLanguage);
} else {
setLanguage(newLanguage);

// load the translations dynamically, first try the language + territory
const po = newLanguage.replace("-", "_");
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, I would move this logic to a function so the caller is easy to read.

@lslezak lslezak merged commit f83484b into master Nov 28, 2024
7 checks passed
@lslezak lslezak deleted the i18n_web_refactoring branch November 28, 2024 16:13
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.

2 participants