Fix #5652 - Runtime i18n Loading for Theme Translations (no build-time merge for i18n files) - #5653
Conversation
|
Hi @kanasznagyzoltan, thank you for this PR! Since this would probably qualify as an improvement to build process rather than a new user feature, I added it to the 10.0 project to start. However since we're close to the May 22 code freeze, this will likely go towards a later release unless reviews are very quick for it. |
Thank you @lgeggleston |
d049149 to
8143bf2
Compare
f40f41d to
c594b07
Compare
|
Hi @lgeggleston (and @saschaszott who welcomed this PR on Slack) — a gentle nudge on this one. It's now rebased on the current main, CI is fully green, there are no conflicts, and it's an isolated change (+410/−19 across 5 files, only the i18n loader), so it should be low-risk to review. 5-minute test recipe:
Regarding the needs documentation label — happy to write this up; would a short page on the DSpace wiki (under the theming / i18n section) be the right place, or a section in the repo docs? I can have it ready quickly so it doesn't hold up the merge. Even a quick review before Jun 30 would be hugely appreciated. Thanks! |
|
@kanasznagyzoltan: I can definitely put this on my list to take a look, but to be honest I can't guarantee that will be possible by the 30th - and for this change I'd only be equipped to test rather than code review. I only do limited amounts of PR testing at the moment, and I usually try to focus first on either time-sensitive fixes or PRs that already have at least 1 review and need a secondary tester. Thank you for keeping this up to date, and I'm aware it's frustrating to not have reviews yet - the large backlog of PRs without reviewers is unfortunately one of the biggest struggles DSpace has been dealing with (and on that note, all the testing and reviewing you and your team have been doing is deeply appreciated!). I can only give the usual recommendations for this - you are welcome to post in the DSpace Slack, or ask another developer to Trade reviews. Second - thanks for being ready to write up a little documentation. The DSpace wiki would be the right place; one place it could work is in this section, but wherever within the wiki you feel it makes sense is likely good! |
|
@lgeggleston In the meantime, do you happen to know whether anyone has been able to take a look at the PR since? Even high-level feedback on the approach itself would be very welcome — I'm happy to adjust based on it. |
…18n-Loading-for-Theme-Translations
|
Hi @lgeggleston, Update — this is now merged with current I've also corrected the PR description: an earlier revision loaded every configured theme, which let sibling themes' keys overwrite one another, so it now loads only the active theme's Short recap for anyone picking this up: it makes the i18n There's a quick (5-10 minute) testing recipe in my comment above, and I'm happy to walk anyone through it on Slack or to adjust the approach if reviewers would prefer something different. |
|
Thank you @kanasznagyzoltan! I tried this today with the following configuration:
Note that I used the So this is +1 from me by testing. I have not reviewed the code. |
Thank you, @alanorth — much appreciated, and good to have the default If you still have it running, would you mind also trying a theme that |
|
Sorry @kanasznagyzoltan, the only site with parent and child themes I have access to is running DSpace 8 so I can't test the |
References
Description
Make the i18n
TranslateLoadertheme-aware: instead of relying on the build-timemerge-i18nscript, theme-specific translation overrides are loaded and merged at runtime fromassets/<theme>/i18n/<lang>.json5, following the active theme'sextendschain. This lets a single Docker image serve multiple customers/themes with their own translations.Instructions for Reviewers
Changes in this PR:
src/ngx-translate-loaders/translate-server.loader.ts— After reading the base i18n file (dist/.../assets/i18n/<lang>.<hash>.json), the loader overlays the active theme's override files (dist/.../assets/<theme>/i18n/<lang>.json5) on top, in inheritance order (root ancestor first, active theme last, so the active theme's keys win). A malformed theme file is skipped with a warning, so it cannot break SSR. The merged map is stored inNGX_TRANSLATE_STATE, so the browser does not re-fetch theme overrides after SSR.src/ngx-translate-loaders/translate-browser.loader.ts— When the TransferState is empty (CSR / dev mode), the loader fetches the base i18n file plus the active theme chain's override.json5files over HTTP and merges them client-side. Missing override files are tolerated viacatchError.src/ngx-translate-loaders/theme-i18n.util.ts(new) — SharedresolveActiveThemeChain()helper used by both loaders. It walks the active theme'sextendschain up to its root ancestor and returns the theme names in ancestor → descendant order, with cycle protection. Sibling themes that are configured but not active are deliberately excluded: loading every configured theme allowed unrelated themes' keys to silently override the active theme's. This mirrors themerge-i18nCLI, which also merges exactly one theme at a time. The active theme is the default theme config — the entry with noregex,handleoruuidrule.docs/ThemeTranslations.md(new) — Documentation covering where theme translation files go, the merge order, which theme's overrides apply, SSR/CSR behaviour, and the relationship tomerge-i18n. Linked from the README file listing.theme-i18n.util.spec.tscovers the load-order logic (extends chains, active-chain-only behaviour, cycles);translate-browser.loader.spec.tscovers the browser merge (TransferState short-circuit, base + theme merge precedence, missing-file fallback). The server loader imports Node'sfs/path, which cannot run in the browser-based Karma suite, so its ordering is covered by the shared-util spec and its merge mirrors the browser path.assets/<theme>/i18n/*.json5files are ignored (missing file → empty object → no effect on the merged result). Existing setups that still runmerge-i18nkeep working unchanged.How it works
Given a config like:
…and i18n files at:
…then with
custom-childas the active theme, the loader merges them at runtime in the orderbase → custom-parent → custom-child, socustom-childkeys overridecustom-parentkeys, which override base keys. Ifcustom-childdoes not define a key, it falls back tocustom-parent's value, then to base.How to test
src/assets/i18n/<lang>.json5contains only base content (no merged theme content), and do not runnpm run merge-i18n.src/themes/<theme>/assets/i18n/<lang>.json5.npm run build:prod) and serve the SSR output. On the home page, verify that theme-specific keys render the override value, that switching language keeps using the theme override, and that non-overridden keys fall back to the base translation./assets/<theme>/i18n/<lang>.json5fires on first navigation (served fromNGX_TRANSLATE_STATE).npm run start:dev(no SSR) and confirm the loader fetches base + theme files over HTTP and merges them in the browser.extendsanother, override a key only in the child and another only in the parent — both should resolve correctly when the child theme is active.Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.