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: introduce i18n framework #3036

Merged
merged 7 commits into from
Aug 11, 2024

Conversation

julianwachholz
Copy link
Contributor

@julianwachholz julianwachholz commented Jul 11, 2024

Let's re-start efforts to bring Actual Budgets to a broader audience by translating the app into different languages.

Previous work: #205

This PR introduces i18next as internationalization framework to lay the groundwork on getting Actual Budget translated into other languages. It features the following concepts:

  • Text is marked for translation using the t() function or <Trans> component.
  • Translatable strings use natural language (English) - no changes to the string literals is being made.
  • Texts are extracted using i18next-parser into a base en JSON file. A shorthand yarn generate:i18n was added to the package.json.
    • Texts from both desktop-client and loot-core are extracted into the same single file located within desktop-client. This reduces complexity by not having to manage multiple namespaces.
  • During a future release process, other language files sourced from a translation service will be placed in the same folder.
  • At runtime, these JSON message catalogues are loaded with a dynamic import to minimize memory usage of the app.

Other notes:

  • To reduce burden on maintainers with endless typo and punctuation updates, I propose setting up a project with Weblate. Weblate offers free hosting for open source projects and is itself also self-hostable, much in the spirit of Actual Budget. :)
  • Using a crowd sourcing application like above will offload individual translation contributions to a separately managed app. Each release cycle will likely only produce one PR to be merged before the release is made.
  • When merged, this PR will change nothing for existing installations. All newly marked-for-translation strings will simply default to the original English language texts. (with the only exception being my added plural example on the transactions list, this wasn't pluralized previously)

Copy link

netlify bot commented Jul 11, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 3e10c20
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/66b90f860e0bc100083c4cda
😎 Deploy Preview https://deploy-preview-3036.demo.actualbudget.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Jul 11, 2024

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
9 4.81 MB → 4.92 MB (+113.02 kB) +2.29%
Changeset
File Δ Size
node_modules/i18next/dist/esm/i18next.js 🆕 +86.37 kB 0 B → 86.37 kB
node_modules/react-i18next/dist/es/TransWithoutContext.js 🆕 +10.6 kB 0 B → 10.6 kB
node_modules/react-i18next/dist/es/useTranslation.js 🆕 +4.22 kB 0 B → 4.22 kB
node_modules/react-i18next/dist/es/utils.js 🆕 +3.09 kB 0 B → 3.09 kB
node_modules/html-parse-stringify/dist/html-parse-stringify.module.js 🆕 +2.02 kB 0 B → 2.02 kB
node_modules/i18next-resources-to-backend/dist/esm/index.js 🆕 +859 B 0 B → 859 B
node_modules/react-i18next/dist/es/Trans.js 🆕 +816 B 0 B → 816 B
src/i18n.ts 🆕 +656 B 0 B → 656 B
node_modules/react-i18next/dist/es/unescape.js 🆕 +613 B 0 B → 613 B
�vite/dynamic-import-helper.js 🆕 +548 B 0 B → 548 B
node_modules/react-i18next/dist/es/defaults.js 🆕 +495 B 0 B → 495 B
node_modules/void-elements/index.js 🆕 +404 B 0 B → 404 B
node_modules/react-i18next/dist/es/context.js 🆕 +335 B 0 B → 335 B
node_modules/react-i18next/dist/es/initReactI18next.js 🆕 +136 B 0 B → 136 B
node_modules/react-i18next/dist/es/i18nInstance.js 🆕 +113 B 0 B → 113 B
src/components/manager/WelcomeScreen.tsx 📈 +594 B (+16.92%) 3.43 kB → 4.01 kB
src/components/manager/subscribe/Bootstrap.tsx 📈 +265 B (+9.94%) 2.6 kB → 2.86 kB
home/runner/work/actual/actual/packages/loot-core/src/client/update-notification.ts 📈 +45 B (+5.33%) 844 B → 889 B
package.json 📈 +152 B (+5.17%) 2.87 kB → 3.02 kB
home/runner/work/actual/actual/packages/loot-core/src/client/shared-listeners.ts 📈 +286 B (+3.75%) 7.44 kB → 7.72 kB
src/components/transactions/SelectedTransactionsButton.jsx 📈 +220 B (+2.77%) 7.76 kB → 7.98 kB
home/runner/work/actual/actual/packages/loot-core/src/client/actions/budgets.ts 📈 +99 B (+1.81%) 5.34 kB → 5.43 kB
src/components/App.tsx 📈 +49 B (+1.05%) 4.54 kB → 4.59 kB
home/runner/work/actual/actual/packages/loot-core/src/client/actions/notifications.ts 📈 +5 B (+0.99%) 504 B → 509 B
src/components/schedules/ScheduleDetails.jsx 📈 +119 B (+0.40%) 28.85 kB → 28.97 kB
src/components/manager/ConfigServer.tsx 📈 +22 B (+0.39%) 5.57 kB → 5.59 kB
src/components/table.tsx 📈 +46 B (+0.19%) 24.28 kB → 24.32 kB
home/runner/work/actual/actual/packages/loot-core/src/platform/client/fetch/index.browser.ts 📈 +5 B (+0.15%) 3.35 kB → 3.36 kB
home/runner/work/actual/actual/packages/loot-core/src/client/actions/queries.ts 📈 +5 B (+0.06%) 7.95 kB → 7.96 kB
node_modules/react/cjs/react.production.min.js 📈 +4 B (+0.05%) 7.44 kB → 7.45 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/index.js 3.08 MB → 3.19 MB (+112.81 kB) +3.57%
static/js/wide.js 245.93 kB → 246.15 kB (+220 B) +0.09%

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/resize-observer.js 18.37 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/narrow.js 77.55 kB 0%
static/js/usePreviewTransactions.js 790 B 0%
static/js/AppliedFilters.js 27.55 kB 0%
static/js/ReportRouter.js 1.24 MB 0%

Copy link
Contributor

github-actions bot commented Jul 11, 2024

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
1 1.14 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
kcab.worker.js 1.14 MB 0%

@wdpk
Copy link
Contributor

wdpk commented Jul 12, 2024

Each release cycle will likely only produce one PR to be merged before the release is made.

I think maybe this is where utilizing github-actions would be powerful. Some other projects implementing similar crowd-sourcing software had a separately maintained repo, and would download the latest release from the translations repo on build. see https://github.com/FOSSBilling/FOSSBilling/blob/72338356a543f1c68ec6be913fbb7c6ede63e09f/.github/workflows/release-build.yml#L48

the translations themselves never touch the main repo. the framework and all the translation calls are obviously in the main repo, but the translations only end up in built versions. when new strings are added to edge, github-actions could also push any new strings from any PRs to the language repo and/or the crowd-sourcing tool(see https://github.com/marketplace/actions/github-transifex-actions) and if translators are active they could update the translations before the PR ever makes it into a release, meaning for end users going from release-to-release, they would never see the untranslated strings.

I set the appropriate key using devtools in brave browser, but it seemingly had the wrong namespace in the i18n.ts call;
i18n.ts:8 i18next::backendConnector: loading namespace translation for language de failed failed parsing /i18n/de/translation.json to json setTimeout (async)

Copy link
Contributor

👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review.

@ramielsawy

This comment was marked as spam.

@github-actions github-actions bot removed the Stale label Jul 27, 2024
@julianwachholz
Copy link
Contributor Author

julianwachholz commented Jul 27, 2024

@ramielsawy it was clear that natural language keys are preferred by the project maintainers from reading the previous discussions: #249 (comment) , #497 (comment)

I myself primarily use and prefer gettext in projecrs which heavily relies on natural language as well.

You repeat yourself in your very one-sided arguments which sounds like you asked ChatGPT for some blurbs that heavily favor one side without giving one single thought to the alternative. Half of your AI-generated points are solved with proper tooling. I will agree that the i18next format is heavily lacking however.

I specifically wrote I started work on this again precisely because the project owners don't want to have a maintenance overhead. I hoped this approach in this PR would alleviate these issues and FINALLY get i18n into Actual.

I personally don't care which way it happens but if the approach two years ago went nowhere because of pointless neverending discussions Actual will never have a translated UI. Unfortunately it's a pain in the rear to retroactively prepare projects for translations but there needs to be some collective effort where we can all pull on the same end of the rope?

@MatissJanis
Copy link
Member

@julianwachholz thanks for the call-out on AI.

@ramielsawy lets keep AI out of this conversation please.

Screenshot 2024-07-27 at 13 56 47

@ramielsawy
Copy link

@julianwachholz @MatissJanis I don't have the same grudge against AI, I am here already because I want (with my team) to build AI on top of Actual and contribute back with our paid time to the core of the project :) Also I don't generate the the content using AI, I write my comment first and then let AI reformat, so don't take offense.

If we can get back to the subject, I'd like to point out that the comment here #205 (comment) gave the feeling that the PR was closed due to inactivity not because it's using keys.

So my yes/no question is: are you open to building on top of #205 or are you stuck in using natural language keys? That will make us save money wasted on some code that you would reject anyways.

@MatissJanis
Copy link
Member

Natural language is the way to go. But you can also think of "natural language" as space-separated keys. :)

--

Looked through the code - everything looks good. Only one point I would like more clarity on:

Translations can be loaded via HTTP and only for the currently selected language.

Is this when building the source? Or would users of the app would need to make an external API call to weblate (or similar service)?

If it's an external API calls for all users - lets not do that. We want the app to be usable offline + without all the trackers that the external 3rd party might add when you query it.

Instead we can pull the translations on build time. Pull them in as JSONs (via HTTP or via github actions automatic sync or another method) and then publish all these JSONs in the final build. And then the user can lazy-load the specific language they use.

@julianwachholz
Copy link
Contributor Author

julianwachholz commented Jul 27, 2024

Thank you for your time to look at this @MatissJanis!

Translations can be loaded via HTTP and only for the currently selected language.

Is this when building the source? Or would users of the app would need to make an external API call to weblate (or similar service)?

I need to revise the original comment, I've since changed this to load the message catalogue with a dynamic import. It was loading from the /public folder before previously, not an external service.
I still need help with adding a dynamic import for messages from loot-core, I could not get that one to work yet.

One big alternate solution that I saw was creating compiled translated sources for each locale, I think Paperless-ngx does this. With this they are able to leverage XLIFF translation files which enable richer translation source catalogues annotated with source location, comments etc. Loading and parsing a big XML file at runtime is a non-starter in my opinion though and compiling different source trees for each locale is a completely new can of worms.

@julianwachholz julianwachholz force-pushed the i18n-preparation branch 5 times, most recently from a9e2c89 to 663c33c Compare July 29, 2024 10:34
namespaceSeparator: false,
defaultValue: (locale, ns, key, value) => {
if (locale === 'en') {
return value || key;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to pre-fill the base en language file with exactly the same content as in the source.

default:
return `An unknown error occurred: ${error}`;
return t(`An unknown error occurred: {{error}}`, { error });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basic interpolation example.

</Link>{' '}
in a new tab for some guidance on what to do when you’ve set your
password.
<Trans>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example using React components. The translatable string will contain placeholders like <2>our tour</2>

const [menuOpen, setMenuOpen] = useState(null);
const triggerRef = useRef(null);

if (selectedItems.size === 0) {
return null;
}

const label = {
transactions: t('{{count}} transactions', { count: selectedItems.size }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example with pluralization.

Using natural language keys requires the base en translation catalogue to exist to be able to also use plural forms in English. Example

@julianwachholz julianwachholz marked this pull request as ready for review July 29, 2024 17:04
@actual-github-bot actual-github-bot bot changed the title [WIP] feat: introduce i18n framework feat: introduce i18n framework Jul 29, 2024
jfdoming
jfdoming previously approved these changes Aug 7, 2024
MatissJanis
MatissJanis previously approved these changes Aug 7, 2024
MikesGlitch
MikesGlitch previously approved these changes Aug 11, 2024
Copy link
Contributor

@MikesGlitch MikesGlitch left a comment

Choose a reason for hiding this comment

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

Very nice :shipit:

There's a merge conflict, can you fix? @julianwachholz

@jfdoming jfdoming merged commit 8142dd1 into actualbudget:master Aug 11, 2024
19 checks passed
@julianwachholz julianwachholz deleted the i18n-preparation branch August 12, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants