Init I18n before Notifications start #23902

maryia-lapata · 2018-10-08T08:47:19Z

Issue: Since Notifications are rendered before i18n engine initializing with specified locale, labels in Notifications are not localized with the specified locale, they are displayed in default locale.

Solution: initialize i18n engine before Notifications are rendered.

Fixed: #23903

With this fix there will be no need for wrapping constants with translated labels into a function in client code. Like I'm trying to make translations to work here and here
In addition I found out that these, these and these translations are currently broken without this fix.

…ialized in CoreSystem

maryia-lapata · 2018-10-15T14:41:54Z

src/core/public/core_system.ts

      const injectedMetadata = this.injectedMetadata.start();
+      this.i18nService.start({ injectedMetadata });


Initialization of i18n engine should be before Notifications start.

Looks like we don't need src/ui/public/chrome/api/translations.js anymore, can you please remove it (including its usage in src/ui/public/chrome/chrome.js)?

And I believe usage of i18n.init in src/ui/ui_render/bootstrap/app_bootstrap.js (and hence usage of await server.getUiTranslations() in src/ui/ui_render/ui_render_mixin.js) isn't not needed anymore as well. Can you please double check and remove everything redundant where possible? With this we'll have just 2 places where we init i18n - one for server and one for client.

@azasypkin sure.
Done.

maryia-lapata · 2018-10-15T14:43:42Z

src/core/public/i18n/i18n_service.ts

+  injectedMetadata: InjectedMetadataStartContract;
+}
+export class I18nService {
+  public start({ injectedMetadata }: Deps) {


This method isn't actually a start method, but I named as start to be consistent with other services.

Maybe at some point we'll do more than just i18n.init, so I'm +1 for consistency.

maryia-lapata · 2018-10-15T14:47:11Z

src/ui/public/i18n/index.js

@@ -28,7 +27,4 @@ import {
 uiModules.get('i18n')
  .provider('i18n', I18nProvider)
  .filter('i18n', i18nFilter)
-  .directive('i18nId', i18nDirective)
-  .config((i18nProvider) => {
-    i18nProvider.init(metadata.translations);


I removed initialization of i18n from here, because it will be already initialized at very beginning of loading application (https://github.com/elastic/kibana/pull/23902/files#diff-ac3204e754680bbed67c8193da8cc11bR103)

azasypkin

LGTM, just a bunch of nits here and there.

@spalger could you please let us know if this PR looks good to you (it touches CoreSystem)?

azasypkin · 2018-10-16T10:07:16Z

packages/kbn-i18n/src/index.ts

@@ -22,3 +22,4 @@ import * as loader from './loader';

 export const i18n = i18nCore;
 export const i18nLoader = loader;
+export { PlainMessages } from './messages';


note: We'll eventually rename PlainMessages to something that is clearer in one of the upcoming PRs (I believe there is one is ready for review already).

src/core/public/i18n/i18n_service.ts

azasypkin · 2018-10-16T10:19:34Z

src/core/public/i18n/i18n_service.ts

+  injectedMetadata: InjectedMetadataStartContract;
+}
+export class I18nService {
+  public start({ injectedMetadata }: Deps) {


Maybe at some point we'll do more than just i18n.init, so I'm +1 for consistency.

azasypkin · 2018-10-16T10:22:17Z

src/core/public/i18n/i18n_service.test.ts

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+const mockI18n = jest.fn();


nit: same nit about new lines here as well, + new line before beforeEach, it etc.

azasypkin · 2018-10-16T10:24:01Z

src/core/public/i18n/i18n_service.test.ts

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+const mockI18n = jest.fn();


nit: maybe mockI18nInit to make it clearer what we actually mock.

azasypkin · 2018-10-16T10:25:17Z

src/core/public/i18n/i18n_service.test.ts

+    const i18nService = new I18nService();
+    expect(mockI18n).not.toHaveBeenCalled();
+    i18nService.start({ injectedMetadata });
+    expect(mockI18n.mock.calls).toMatchSnapshot();


nit: for such small snapshots I'd better use toMatchInlineSnapshot or even better something like this:

expect(mockI18nInit).toHaveBeenCalledTimes(1); expect(mockI18nInit).toHaveBeenCalledWith(translations);

Thank you, done.

azasypkin · 2018-10-16T10:26:43Z

src/core/public/core_system.ts

      const injectedMetadata = this.injectedMetadata.start();
+      this.i18nService.start({ injectedMetadata });


Looks like we don't need src/ui/public/chrome/api/translations.js anymore, can you please remove it (including its usage in src/ui/public/chrome/chrome.js)?

src/core/public/i18n/index.ts

azasypkin · 2018-10-16T10:40:27Z

src/core/public/core_system.ts

      const injectedMetadata = this.injectedMetadata.start();
+      this.i18nService.start({ injectedMetadata });


And I believe usage of i18n.init in src/ui/ui_render/bootstrap/app_bootstrap.js (and hence usage of await server.getUiTranslations() in src/ui/ui_render/ui_render_mixin.js) isn't not needed anymore as well. Can you please double check and remove everything redundant where possible? With this we'll have just 2 places where we init i18n - one for server and one for client.

…pBootstrap were changed

elasticmachine · 2018-10-17T08:59:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2018-10-18T00:57:49Z

src/core/public/i18n/i18n_service.ts

+
+export class I18nService {
+  public start({ injectedMetadata }: Deps) {
+    i18n.init(injectedMetadata.getLegacyMetadata().translations);


It took me a while to track this down and figure out how everything was taking advantage of the translations without consuming the new platform at all, turns out it's because we are just using new platform constructs to store the translations in @kbn/i18n module. Based on this I don't think the I18nService is necessary, and I don't really think this does what we want for the new platform.

I'm down to move this to the new platform, but if we are going to do that then we should be instantiating something that will wrap the translations, and exposing that on the I18nService start contract, not by caching the state in @kbn/i18n.

In order to address https://github.com/elastic/kibana/pull/23603/files#r225105739 all we would really need to do is move the call to i18n.init() out of an angular .config() function, and we can just do that by adding it to src/ui/public/i18n/index.js for now until we want to move @kbn/i18n fully to the new platform:

diff --git a/src/ui/public/chrome/chrome.js b/src/ui/public/chrome/chrome.js index cfae977feb..fe0cc3011f 100644 --- a/src/ui/public/chrome/chrome.js +++ b/src/ui/public/chrome/chrome.js @@ -21,6 +21,7 @@ import _ from 'lodash'; import angular from 'angular'; import { metadata } from '../metadata'; +import '../i18n'; import '../state_management/global_state'; import '../config'; import '../notify'; diff --git a/src/ui/public/i18n/index.js b/src/ui/public/i18n/index.js index 5def86410e..1f4b435f61 100644 --- a/src/ui/public/i18n/index.js +++ b/src/ui/public/i18n/index.js @@ -18,12 +18,16 @@ */ import { uiModules } from 'ui/modules'; +import { metadata } from 'ui/metadata'; +import { i18n } from '@kbn/i18n'; import { I18nProvider, i18nFilter, i18nDirective, } from '@kbn/i18n/angular'; +i18n.init(metadata.translations); + uiModules.get('i18n') .provider('i18n', I18nProvider) .filter('i18n', i18nFilter)

(or something like that)

@spalger The main reason for this PR is that Notifications template is rendered earlier than i18n is initialized with specified locale.
It's important because to make labels, passing in Notifications, translatable we should wrap Notifications template by I18nProvider https://github.com/elastic/kibana/pull/22757/files#diff-53c9b70520405921f846a26c104aeb23R40 which is used to setup the i18n context for a tree. And in the case when we setup the locale in src/ui/public/i18n/index.js, the i18n context for Notifications will be defined with a default locale not counting the data specified in src/ui/public/i18n/index.js.

Here the order of files loading:

That's why I placed i18n initialization just before Notifications template is rendered.

Okay, I didn't see that we were going to start consuming <I18nProvider /> from the new platform. In that case, can we just move the init() call to the entry file so it's initialized before any other code is run? I just don't want to have an i18n service in the new platform unless it's useful, and I don't think this version is really there.

diff --git a/src/ui/ui_bundles/app_entry_template.js b/src/ui/ui_bundles/app_entry_template.js index 764e70eaa1..a74ef61848 100644 --- a/src/ui/ui_bundles/app_entry_template.js +++ b/src/ui/ui_bundles/app_entry_template.js @@ -33,10 +33,15 @@ import 'whatwg-fetch'; import 'abortcontroller-polyfill'; import 'childnode-remove-polyfill'; +import { i18n } from '@kbn/i18n'; import { CoreSystem } from '__kibanaCore__' +const injectedMetadata = JSON.parse(document.querySelector('kbn-injected-metadata').getAttribute('data')); +i18n.init(injectedMetadata.legacyMetadata.translations); + new CoreSystem({ - injectedMetadata: JSON.parse(document.querySelector('kbn-injected-metadata').getAttribute('data')), + injectedMetadata: injectedMetadata, rootDomElement: document.body, requireLegacyFiles: () => { ${bundle.getRequires().join('\n ')}

elasticmachine · 2018-10-19T12:37:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

LGTM, I don't have any translations to test, but the logic looks good!

maryia-lapata · 2018-10-19T13:45:46Z

@spalger I've checked this code with translations.

* Init I18n before Notifications start * Remove i18n initialization in AngularJS since it will be already initialized in CoreSystem * Remove unused translationsApi * Remove redundant i18n.init invocation * Remove i18n tests since i18n.init was removed and input params for AppBootstrap were changed * Move i18n initialization to the entry file.

Init I18n before Notifications start

e9b502d

maryia-lapata added the Project:i18n label Oct 8, 2018

maryia-lapata self-assigned this Oct 8, 2018

maryia-lapata requested review from azasypkin and pavel06081991 October 8, 2018 08:47

Export message type from kbn-i18n

4ebd4fe

azasypkin added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Oct 8, 2018

Remove i18n initialization in AngularJS since it will be already init…

797c43b

…ialized in CoreSystem

maryia-lapata mentioned this pull request Oct 9, 2018

Translations for Reporting plugin #22757

Merged

elastic deleted a comment from elasticmachine Oct 9, 2018

Merge branch 'master' into fix-i18n-init

b50d4dd

elastic deleted a comment from elasticmachine Oct 10, 2018

elastic deleted a comment from elasticmachine Oct 12, 2018

This was referenced Oct 15, 2018

[Rollups] Rollup support in Kibana, phase 1 #21117

Merged

Translate vis color schema labels #23603

Merged

Rename method

9523721

maryia-lapata commented Oct 15, 2018

View reviewed changes

maryia-lapata requested a review from LeanidShutau October 15, 2018 14:44

maryia-lapata commented Oct 15, 2018

View reviewed changes

Remove usused translationsApi

7282ab2

azasypkin approved these changes Oct 16, 2018

View reviewed changes

azasypkin requested a review from spalger October 16, 2018 10:44

Remove redundant i18n.init invocation

6835ea8

maryia-lapata added 2 commits October 16, 2018 16:48

Refactoring

7e2071a

Remove i18n tests since i18n.init was removed and input params for Ap…

87f4b67

…pBootstrap were changed

elastic deleted a comment from elasticmachine Oct 16, 2018

elastic deleted a comment from elasticmachine Oct 17, 2018

Merge branch 'master' into fix-i18n-init

a773b20

elastic deleted a comment from elasticmachine Oct 17, 2018

spalger suggested changes Oct 18, 2018

View reviewed changes

maryia-lapata added 4 commits October 18, 2018 11:42

Merge branch 'master' into fix-i18n-init

c039583

Merge branch 'master' into fix-i18n-init

e2bc5d4

Move i18n initialization to the entry file.

fb989d3

Refactoring

1823f91

spalger approved these changes Oct 19, 2018

View reviewed changes

elastic deleted a comment from elasticmachine Oct 19, 2018

maryia-lapata merged commit 4b9e13c into elastic:master Oct 19, 2018

maryia-lapata deleted the fix-i18n-init branch October 19, 2018 14:59

maryia-lapata added the backport pending label Oct 19, 2018

maryia-lapata mentioned this pull request Oct 19, 2018

[6.x] Init I18n before Notifications start (#23902) #24267

Merged

maryia-lapata added backported and removed backport pending labels Oct 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init I18n before Notifications start #23902

Init I18n before Notifications start #23902

maryia-lapata commented Oct 8, 2018 •

edited

Loading

maryia-lapata Oct 15, 2018

azasypkin Oct 16, 2018

azasypkin Oct 16, 2018

maryia-lapata Oct 16, 2018

maryia-lapata Oct 15, 2018

azasypkin Oct 16, 2018

maryia-lapata Oct 15, 2018

azasypkin left a comment

azasypkin Oct 16, 2018

azasypkin Oct 16, 2018

azasypkin Oct 16, 2018

azasypkin Oct 16, 2018

azasypkin Oct 16, 2018

maryia-lapata Oct 16, 2018

azasypkin Oct 16, 2018

azasypkin Oct 16, 2018

elasticmachine commented Oct 17, 2018

spalger Oct 18, 2018 •

edited

Loading

maryia-lapata Oct 18, 2018

spalger Oct 18, 2018

elasticmachine commented Oct 19, 2018

spalger left a comment

maryia-lapata commented Oct 19, 2018

		const injectedMetadata = this.injectedMetadata.start();
		this.i18nService.start({ injectedMetadata });

Init I18n before Notifications start #23902

Init I18n before Notifications start #23902

Conversation

maryia-lapata commented Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 17, 2018

💚 Build Succeeded

spalger Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 19, 2018

💚 Build Succeeded

spalger left a comment

Choose a reason for hiding this comment

maryia-lapata commented Oct 19, 2018

maryia-lapata commented Oct 8, 2018 •

edited

Loading

spalger Oct 18, 2018 •

edited

Loading