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

[Qol][Refactor] i18n lazy-loading #4327

Open
wants to merge 21 commits into
base: beta
Choose a base branch
from

Conversation

flx-sta
Copy link
Collaborator

@flx-sta flx-sta commented Sep 19, 2024

Caution

Make sure to consult the translation team before merging as all locale files moved from /src into /public !!!

What are the changes the user will see?

Only the languages being used will be loaded thus less memory usage from the i18n side

Why am I making these changes?

The memory shouldn't be bloated with all languages at all time. IT should only hold the languages currently being used which at max is 2 (en as a fallback for missing keys).

What are the changes from a developer perspective?

  1. Moved all /src/locales into /public/locales as they will be loaded async (via network request).
  2. Install the i18next-http-backend plugin which is necessary for lazy-loading
  3. Add a camelCaseToKebabCase function to utils
  4. Clean up /src/plugin/i18n.ts file structure
  5. move phaser config into startGame() and lazy-load the scenes to prevent any i18n.t() usages before it is initialized
  6. Remove initI18n() from LoadingScene
  7. Remove all config.ts files inside the locales/
  8. Remove all locales tests. Reason: We don't need to test if i18next is working properly. They have their own test-coverage to ensure that
  9. i18next is now mocked for tests. This was overdue and now necessary due to the backend module

How to test the changes?

  1. Play the game
  2. Switch languages and keep on playing.
  3. Check the network tab for any failed translation requests (locales/**)

Checklist

  • I'm using beta as my base branch
  • There is no overlap with another PR?
  • The PR is self-contained and cannot be split into smaller PRs?
  • Have I provided a clear explanation of the changes?
  • [ ] Have I considered writing automated tests for the issue?
  • [ ] If I have text, did I make it translatable and add a key in the English locale file(s)?
  • Have I tested the changes (manually)?
    • Are all unit tests still passing? (npm run test)
  • [ ] Are the changes visual?
    • [ ] Have I provided screenshots/videos of the changes?

@flx-sta
Copy link
Collaborator Author

flx-sta commented Sep 19, 2024

Review Helper

to mark all **/*.json files as Viewed in the Files tab simply run this script in your browser-console:

 $$("#files .js-reviewed-toggle > input[id*='.json']").forEach((el) => el.click()); 

Your browser will probably lagg for 1-3 minutes as it sends quite a lot of server-requests at the same time then

src/test/vitest.setup.ts Outdated Show resolved Hide resolved
src/test/vitest.setup.ts Outdated Show resolved Hide resolved
@DayKev DayKev added Localization Provides or updates translation efforts Miscellaneous Changes that don't fit under any other label Refactor Rewriting existing code related labels Sep 19, 2024
now using "mysteryEncounters/..."
the encounter was checking if the modifier name includes `Berry` which is only true for english. Instead it has to check if the modifier is an instance of BerryModifier
the new i18n pattern requires a different namespacing which has been adopted
@flx-sta flx-sta marked this pull request as ready for review September 20, 2024 00:23
@flx-sta flx-sta requested review from a team as code owners September 20, 2024 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Localization Provides or updates translation efforts Miscellaneous Changes that don't fit under any other label Refactor Rewriting existing code related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants