I18n: Migrate to I18next #55845

joshhunt · 2022-09-27T15:09:00Z

What this PR does / why we need it:

The outcome from the work done on #55300 was that we want to switch to a translation library that has less dependencies on our build tooling.

This PR switches Grafana to I18next to remove our dependency on babel-macros. The tradeoff is:

no dependency on babel or babel macro
- this also simplifies things for testing
slightly less ergonomic API for interpolating values in strings and components. Rather than just using JSX and/or template string interpolation, you must use i18next/react-i81next specific syntax.
- There is a caveat around using this with HTML components in React 18 Cannot pass objects to <Trans> with React 18 type definitions i18next/react-i18next#1483
the default english phrase is left in the js bundle in components, rather than being stripped out. When you use a language other than en-US, you'll be loading the english phrases in the js bundle, and then the translations will load async after that.
- The upside of this is that, with some extra fiddling around with tooling, you can update the english phrase in the JSX source, rather than needing to update the messages catalogue
- Another option we have is to not include any english phrases in our JSX source, and require developers to manually add them to the messages.json message catalogues. I don't think this is worth it.

Which issue(s) this PR fixes:

Part of #56303

Special notes for your reviewer:

Many files are changed in this PR to switch messages from lingui to i18next, but I've tried to make the list of commits more approachable way to review this. The main changes to scrutinise is the first "Switch from lingui from i18next" commit

TODO:

Make sure tests work
Verify all lingui-style t() template strings are using the correct i18next interpolation syntax
Verify all lingui-style Trans JSX interpolation is using the correct i18next interpolation syntax
Verify it works as expected with i18n feature flag disabled.
- Verify navigation translations works as expected
How do we compile the messages .json files? 'default phrase' will remain in the source code, so do we just not need to load en-us messages? Should we not commit the en-us messages json? How will crowdin get the english messages?
- will tackle this in separate pr focusing on crowdin

In later PRs we will

update our contributing documentation to reflect this change
add in some eslint rules to make sure the right thing is imported from the right place
extra tooling to help with extracting and syncing with crowdin

grafanabot · 2022-09-27T15:14:04Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/35174

grafanabot · 2022-09-29T14:31:22Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/35589

grafanabot · 2022-09-29T15:28:15Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/35605

joshhunt · 2022-09-30T08:22:22Z

packages/grafana-ui/src/utils/i18n.tsx

+export const t = (id: string, defaultMessage: string) => {
+  initI18n();
+  return i18next.t(id, defaultMessage);
+};


We will add a lint rule to ensure that people import these components when translating grafana-ui

grafanabot · 2022-09-30T08:41:32Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/35670

JoaoSilvaGrafana

This all looks really good, nice job in switching all those translated places from lingui to i18next!
I've got one question though, it seems like we are importing things from various different places, like:
import { Trans } from 'react-i18next';
import { t } from 'app/core/internationalization';
import { t } from 'i18next';
import { useTranslation } from 'react-i18next'; and then const { t } = useTranslation();
import { t, Trans } from '../../../utils/i18n'; when in grafana-ui

Is there a reason why we can't use the util from grafana-ui or a similar one everywhere?

JoaoSilvaGrafana · 2022-09-30T10:38:28Z

public/locales/psuedo.js

@@ -0,0 +1,17 @@
+const fs = require('fs/promises');


There is a typo on this file name psuedo.js

JoaoSilvaGrafana · 2022-09-30T10:51:22Z

packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimePickerContent.tsx

@@ -353,7 +359,7 @@ const getEmptyListStyles = stylesFactory((theme: GrafanaTheme2) => {
      a,
      span {
        font-size: 13px;
-      }
+      }\


Was this intended? 🤔

joshhunt · 2022-09-30T11:51:30Z

yup - good catch @JoaoSilvaGrafana. There's still a bit of tidy-up work i need to do (i switched where i was importing half way through updating all the lingui stuff), but I was wondering the exact same thing as you - can we centralize all the i18n setup and imports into grafana/ui?

I would probably aim to move all of app/core/internationalisation into grafana-ui, along with the message catalogues. Would need to make sure that the translations are not included in the standalone bundle, and would need to stress this is not a supported/public API.

My one thought/concern is whether this would create some awkward version dependency things for us. @jackw do you have any thoughts on this?

lpskdl · 2022-09-30T11:56:33Z

public/app/app.ts

+        loadLocalePromise,
+
+        // Preload selected app plugins
+        await preloadPlugins(config.pluginsToPreload),


do we need to use await for this promise?

no! good catch!

joshhunt · 2022-10-04T12:43:58Z

@mckn all three of these questions (and my answers) are tightly related.

The main thing I'm optimising for here are the many other developers (at grafana and in the community!) who get subjected to this when developing UIs to support translation. The harder we make it to support translation, the worse of a job they will do and the more inconsistent i18n would be in Grafana. The goal is to make translating grafana as simple as possible, using workflows as close to their current one, so more of Grafana is translated in a higher quality. We should use tooling to automate the busy work to let developers focus on the real stuff.

Given how many files we have changed only to move from one library to another. Does it makes sense to add a small abstraction (which I believe you have done in grafana-ui) so we can swap library under the hood?

The justification for this is tricky. Because we statically extract the strings (with both lingui and i18next), introducing a wrapper component complicates this. i18next-parser is less strict than lingui's and it only looks at the name of the function/component, but still the name of the props are important.

However, as Joao mentioned in his comment, the imports in this PR are inconsistent and need to be standardised. I think what I'm going to do is to create wrapper components in grafana core, the same as grafana-ui, and use lint rules to enforce that the correct imports are used. I'm extremely hesitant to put too much of the actual i18n framework in grafana-ui - I'm cautious to create weird version dependencies issues between grafana and grafana-ui, and plugins (somehow).

I'm not a big fan of the whole "default" value at call site. [...] So what you end up with is basically needing to manage english (in this case) translations in multiple places.

I agree. Having english phrases in multiple places is a no-go.

The goal I have in mind (which would probably do in the next step to prepare for integrating with crowdin) would be that the english messages.json file would be empty, and the english phrases would only be in the source components. I think this is the easiest model for developers - phrases are where they expect them (they're in-situ, just as if there's no translation) and they don't need to manually add keys and messages into another file. What's your thoughts on that - i think it resolves that concern directly?

Would it be possible to extract typings from the translation files to minimize the risk of passing an invalid key to the translate functions.

For context - key/phrase reuse is discouraged. The public contributing docs mention this very passively in passing (needs to be fixed!), but from previous conversations we established the rule of thumb to not reuse a phrase.

At the moment, because messages are statically extracted from the call site, the call site is the source of truth for translation keys. We don't need to typecheck this because it's impossible for it to be wrong. Most of the time when you write a component, the key won't exist yet, so if we were typechecking this the default experience would be type errors, which I think isn't ideal.

I think statically extracting phrases from the source code is the best approach. It's easier for developers and removes busy work from the process of creating new UI that supports i18n. It also ensures translations are most correct and error free, in regard to interpolation.

When we use static extraction, it makes sure the extracted phrase in all locale's messages catalogues have the correct variable name and component order:

<Trans i18nKey="page.title">Hello {{username}}</Trans>

// extracted to messages.json
{
"page.title": "Hello {{username}}",
}

If we were added these phrases into the JSON manually, we would need to make sure that the variable name pass in the values object matches the phrase in all the message catalogues. Remember, these are all a bunch of JSON files, so there's no great way to typecheck/validate the contents of all the phrases:

<Trans i18nKey="page.title" values={{username}} />

// manually update messages.json
{
"page.title": "Hello {{username}}", // make sure the username variable matches the source across all locales
}

This gets even trickier when doing JSX interpolation

<Trans
  i18nKey="page.title"
  values={{ username }}
  components={[<strong />, <em />]} // make sure these components are in the correct order as the phrases across all locales
/>

// manually update messages.json
{
"page.title": "<0>Hello</0> <1>{{username}}</1>", // make sure the username variable matches the source across all locales, ane
}

If we don't automate this by statically extracting the phrases, I think we will have a much higher chance of incorrect and inconsistent translations. I think statically extracting message keys and strings from our source gives us the most consistency and correctness.

Thoughts @mckn?

jackw · 2022-10-04T13:04:52Z

I would probably aim to move all of app/core/internationalisation into grafana-ui, along with the message catalogues. Would need to make sure that the translations are not included in the standalone bundle, and would need to stress this is not a supported/public API.

Isn't this gonna be tricky as anything in grafana-ui is public? 🤔

My one thought/concern is whether this would create some awkward version dependency things for us. @jackw do you have any thoughts on this?

Not sure this will create an awkward version dependency. Maybe I'm missing something so please expand on that if you think it's a concern. We need to make sure that there is only ever one instance running otherwise I think we'll lose the context, right?

joshhunt · 2022-10-04T13:09:11Z

@jackw I would put it in grafana-ui as a "private api" (if such exists - just not export it from the root index.js?) where we would not support it and have breaking changes at any time.

I can't think of any specific problems - on the surface it seems fine, but I know how we've had related headaches to this before so I'm trying to be very cautious around this!

grafanabot · 2022-10-04T13:49:30Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/36107

grafanabot · 2022-10-04T14:56:50Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/36129

joshhunt · 2022-10-04T15:20:45Z

Hmmm, the <Trans>Hello <strong>{{username}}</strong></Trans> syntax is problematic with React 18 with HTML children because React children types have been updated to disallow objects. See i18next/i18next-parser#603 and i18next/react-i18next#1483

There's a rather blunt fix/workaround to use module augmentation to use the old HTMLAttribute children type, or we would have to fall back to the alternate syntax which is a bit more problematic due to the reasons i mentioned in my above comment.

The more this goes on, the more I'm feeling that we're putting too much weight on simplifying a build tooling issue by sacrificing ergonomics for all the developers. Like @academo said:

In general any approach that relies on developers passing the right variable that comes from somewhere to somewhere else in a repetitive way sounds like putting the responsibility of a build system to the developer.

mckn · 2022-10-05T11:52:20Z

I agree. Having english phrases in multiple places is a no-go.

The goal I have in mind (which would probably do in the next step to prepare for integrating with crowdin) would be that the english messages.json file would be empty, and the english phrases would only be in the source components. I think this is the easiest model for developers - phrases are where they expect them (they're in-situ, just as if there's no translation) and they don't need to manually add keys and messages into another file. What's your thoughts on that - i think it resolves that concern directly?

For context - key/phrase reuse is discouraged. The public contributing docs mention this very passively in passing (needs to be fixed!), but from previous conversations we established the rule of thumb to not reuse a phrase.

At the moment, because messages are statically extracted from the call site, the call site is the source of truth for translation keys. We don't need to typecheck this because it's impossible for it to be wrong. Most of the time when you write a component, the key won't exist yet, so if we were typechecking this the default experience would be type errors, which I think isn't ideal.

I think statically extracting phrases from the source code is the best approach. It's easier for developers and removes busy work from the process of creating new UI that supports i18n. It also ensures translations are most correct and error free, in regard to interpolation.

When we use static extraction, it makes sure the extracted phrase in all locale's messages catalogues have the correct variable name and component order:
<Trans i18nKey="page.title">Hello {{username}}</Trans>

// extracted to messages.json
{
"page.title": "Hello {{username}}",
}
If we were added these phrases into the JSON manually, we would need to make sure that the variable name pass in the values object matches the phrase in all the message catalogues. Remember, these are all a bunch of JSON files, so there's no great way to typecheck/validate the contents of all the phrases:
<Trans i18nKey="page.title" values={{username}} />

// manually update messages.json
{
"page.title": "Hello {{username}}", // make sure the username variable matches the source across all locales
}
This gets even trickier when doing JSX interpolation
<Trans
  i18nKey="page.title"
  values={{ username }}
  components={[<strong />, <em />]} // make sure these components are in the correct order as the phrases across all locales
/>

// manually update messages.json
{
"page.title": "<0>Hello</0> <1>{{username}}</1>", // make sure the username variable matches the source across all locales, ane
}
If we don't automate this by statically extracting the phrases, I think we will have a much higher chance of incorrect and inconsistent translations. I think statically extracting message keys and strings from our source gives us the most consistency and correctness.

Thoughts @mckn?

Yes, when I'm thinking about this I really like the approach here! Great stuff!

One thing that I was thinking about regarding the utility functions in grafana-ui. Would it be possible to expose a Provider so consumers of Grafana UI need to use that when consuming components from grafana-ui?

joshhunt · 2022-10-05T12:24:06Z

Would it be possible to expose a Provider so consumers of Grafana UI need to use that when consuming components from grafana-ui?

@mckn Requiring a provider to use Grafana-UI would be a breaking change, where the goal of translating grafana-ui is to do it in a way that is not a breaking change. I see no benefit from using a provider at the moment.

mckn · 2022-10-05T12:33:14Z

Would it be possible to expose a Provider so consumers of Grafana UI need to use that when consuming components from grafana-ui?

@mckn Requiring a provider to use Grafana-UI would be a breaking change, where the goal of translating grafana-ui is to do it in a way that is not a breaking change. I see no benefit from using a provider at the moment.

Yeah - makes sense but at the same time you won't be able to use the localization functionality of the grafana/ui components. But that might be ok until we have made som more progress on this.

joshhunt · 2022-10-05T12:42:53Z

@mckn Yup at the moment we are not supporting external consumers (gcom) to use the translations. At the moment these are only available within Grafana.

I don't think this takes us down any dead ends that would make supporting this difficult in the future, and the wrappers in app/core/i18n and grafana-ui/utils/i18n would probably make this easier for us later.

JoaoSilvaGrafana

Seems pretty similar to Lingui so I'd say easy to use from a developer's perspective. Only thing I'm curious about is how are plurals done in i18next, but regardless of that I'd say we go with the workaround to get this to work with React 18 and it looks good to me otherwise 👍

lpskdl

LGTM 🚀 . I have pretty much the same feedback as @JoaoSilvaGrafana also the comparison document is fair regarding the tradeoffs between using lingui over react-i18n.

grafanabot · 2022-10-06T12:44:15Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/36514

grafanabot added area/frontend enterprise-failed labels Sep 27, 2022

joshhunt mentioned this pull request Sep 28, 2022

Build: Reintroduce babel for grafana-ui #55300

Closed

joshhunt force-pushed the joshhunt/i18next branch from 7fa249d to 686bae1 Compare September 29, 2022 14:16

joshhunt force-pushed the joshhunt/i18next branch from 357e429 to a9e5687 Compare September 29, 2022 15:15

joshhunt requested a review from jackw September 29, 2022 15:36

joshhunt marked this pull request as ready for review September 29, 2022 15:36

joshhunt requested a review from a team as a code owner September 29, 2022 15:36

joshhunt requested a review from a team September 29, 2022 15:36

joshhunt requested review from a team as code owners September 29, 2022 15:36

joshhunt requested review from davkal, dprokop, ashharrison90 and JoaoSilvaGrafana and removed request for a team September 29, 2022 15:36

joshhunt added this to the 9.3.0 milestone Sep 29, 2022

joshhunt added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Sep 29, 2022

joshhunt commented Sep 30, 2022

View reviewed changes

joshhunt force-pushed the joshhunt/i18next branch 2 times, most recently from 385fff5 to ba15db8 Compare September 30, 2022 08:30

JoaoSilvaGrafana reviewed Sep 30, 2022

View reviewed changes

lpskdl reviewed Sep 30, 2022

View reviewed changes

joshhunt mentioned this pull request Oct 4, 2022

I18n: Migrate to I18next #56303

Closed

4 tasks

joshhunt force-pushed the joshhunt/i18next branch from ba15db8 to 887c6e0 Compare October 4, 2022 13:31

joshhunt force-pushed the joshhunt/i18next branch from c66c92f to cbf5ab5 Compare October 4, 2022 14:47

JoaoSilvaGrafana approved these changes Oct 5, 2022

View reviewed changes

lpskdl approved these changes Oct 5, 2022

View reviewed changes

joshhunt force-pushed the joshhunt/i18next branch from cbf5ab5 to a6908e2 Compare October 6, 2022 12:23

Switch from lingui from i18next

da3ea19

joshhunt force-pushed the joshhunt/i18next branch from a6908e2 to 6681872 Compare October 6, 2022 12:23

joshhunt added 3 commits October 6, 2022 14:45

Change lingui messages to i18next messages

efeef4b

Change lingui messages to i18next messages (grafana-ui)

c13abe6

Init i18n for tests

ffe075a

joshhunt force-pushed the joshhunt/i18next branch from 6681872 to ffe075a Compare October 6, 2022 13:46

grafanabot added enterprise-ok and removed enterprise-failed labels Oct 6, 2022

joshhunt merged commit 5361efc into main Oct 6, 2022

joshhunt deleted the joshhunt/i18next branch October 6, 2022 15:34

leandro-deveikis modified the milestones: 9.3.0, 9.3.0-beta1 Nov 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I18n: Migrate to I18next #55845

I18n: Migrate to I18next #55845

joshhunt commented Sep 27, 2022 •

edited

Loading

grafanabot commented Sep 27, 2022

grafanabot commented Sep 29, 2022

grafanabot commented Sep 29, 2022

joshhunt Sep 30, 2022

grafanabot commented Sep 30, 2022

JoaoSilvaGrafana left a comment

JoaoSilvaGrafana Sep 30, 2022

JoaoSilvaGrafana Sep 30, 2022

joshhunt commented Sep 30, 2022

lpskdl Sep 30, 2022

joshhunt Sep 30, 2022

joshhunt commented Oct 4, 2022 •

edited

Loading

jackw commented Oct 4, 2022 •

edited

Loading

joshhunt commented Oct 4, 2022

grafanabot commented Oct 4, 2022

grafanabot commented Oct 4, 2022

joshhunt commented Oct 4, 2022 •

edited

Loading

mckn commented Oct 5, 2022

joshhunt commented Oct 5, 2022

mckn commented Oct 5, 2022

joshhunt commented Oct 5, 2022

JoaoSilvaGrafana left a comment

lpskdl left a comment

grafanabot commented Oct 6, 2022

I18n: Migrate to I18next #55845

I18n: Migrate to I18next #55845

Conversation

joshhunt commented Sep 27, 2022 • edited Loading

grafanabot commented Sep 27, 2022

grafanabot commented Sep 29, 2022

grafanabot commented Sep 29, 2022

joshhunt Sep 30, 2022

Choose a reason for hiding this comment

grafanabot commented Sep 30, 2022

JoaoSilvaGrafana left a comment

Choose a reason for hiding this comment

JoaoSilvaGrafana Sep 30, 2022

Choose a reason for hiding this comment

JoaoSilvaGrafana Sep 30, 2022

Choose a reason for hiding this comment

joshhunt commented Sep 30, 2022

lpskdl Sep 30, 2022

Choose a reason for hiding this comment

joshhunt Sep 30, 2022

Choose a reason for hiding this comment

joshhunt commented Oct 4, 2022 • edited Loading

jackw commented Oct 4, 2022 • edited Loading

joshhunt commented Oct 4, 2022

grafanabot commented Oct 4, 2022

grafanabot commented Oct 4, 2022

joshhunt commented Oct 4, 2022 • edited Loading

mckn commented Oct 5, 2022

joshhunt commented Oct 5, 2022

mckn commented Oct 5, 2022

joshhunt commented Oct 5, 2022

JoaoSilvaGrafana left a comment

Choose a reason for hiding this comment

lpskdl left a comment

Choose a reason for hiding this comment

grafanabot commented Oct 6, 2022

joshhunt commented Sep 27, 2022 •

edited

Loading

joshhunt commented Oct 4, 2022 •

edited

Loading

jackw commented Oct 4, 2022 •

edited

Loading

joshhunt commented Oct 4, 2022 •

edited

Loading