Add draft implementation of I18n engine #19555

maksim-tolo · 2018-05-30T15:13:44Z

It's just a draft implementation of i18n engine. I'm going to encapsulate all i18n logic into @kbn/i18n package.

elasticmachine · 2018-05-30T16:18:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin

I haven't run Kibana with this PR yet, but went through code and left several comments that I'd like to discuss/handle first.

Also it looks like a good moment to start adding Jest unit tests for the new code.

azasypkin · 2018-06-01T08:52:48Z

src/ui/ui_i18n/README.md

@@ -107,6 +109,7 @@ when missing translations
 - `getDefaultLocale()` - returns the default locale
 - `defineFormats(formats: object)` - supplies a set of options to the underlying formatter.


nit: now when we have getFormats it would probably make sense to unify naming and rename defineFormats into setFormats. Same for ##Angular section below.

azasypkin · 2018-06-01T09:07:13Z

src/ui/ui_render/ui_render_mixin.js

@@ -148,7 +150,7 @@ export function uiRenderMixin(kbnServer, server, config) {
          injectedVarsOverrides
        }),
        bundlePath: `${config.get('server.basePath')}/bundles`,
-        i18n: key => get(translations, key, ''),
+        i18n: i18n.translate.bind(i18n),


nit: let's use arrow function here instead, it will allow us to quickly see what the translate signature is.

azasypkin · 2018-06-01T09:17:24Z

packages/kbn-i18n/package.json

+  "license": "Apache-2.0",
+  "private": true,
+  "dependencies": {
+    "intl-format-cache": "2.1.0",


nit: let's use ^-versions for all dependencies.

I'm not really sure that's a nit? I would say it's important.

Hmm, yeah, it's, I need some new keyword for important-but-trivial-change: :)

azasypkin · 2018-06-01T09:57:26Z

packages/kbn-i18n/src/angular_i18n/i18n.directive.js

+ * under the License.
+ */
+
+import { uiModules } from 'ui/modules';


issue: hmm, if we make knb-i18n a separate package I'd not expect seeing Kibana specific dependencies here at all. I believe we should either create our own Angular i18n module that will include directive/filter/provider that will be registered with other kibana angular modules or export filter/directive and provider like that and register them manually in KIbana:

// src/angular/filter.js export function filter(i18n) { return function(id, { defaultMessage = '', values = {} } = {}) { ...... }; // src/angular/directive.js export function directive(i18n) { return { restrict: 'A', .... }; };

Thoughts?

I've added src/ui/public/i18n module that registers directive/filter/provider from knb-i18n package.

azasypkin · 2018-06-01T09:59:16Z

packages/kbn-i18n/src/angular_i18n/i18n.servise.js

@@ -0,0 +1,60 @@
+/*


nit: technically it's provider, not angular service (there is typo in file name btw).

azasypkin · 2018-06-01T11:09:50Z

packages/kbn-i18n/src/core.js

+
+const hasValues = values => Object.keys(values).length > 0;
+
+const addLocaleData = localeData => {


nit: please add comment with explanation what this code does exactly and why.

azasypkin · 2018-06-01T11:28:01Z

packages/kbn-i18n/src/core.js

+
+    const message = getMessageById(this.getMessages(), id);
+
+    if (!hasValues(values) && process.env.NODE_ENV === 'production') {


question: why do we need process.env.NODE_ENV === 'production' here? Shouldn't it fail for angular and react?

That's needed to avoid expensive message formatting for simple messages without values. In dev environment messages will always be formatted

Sorry, I don't really get what you mean. Maybe small example?

azasypkin · 2018-06-01T11:31:25Z

packages/kbn-i18n/src/core.js

+
+const EN_LOCALE = 'en';
+
+const getMessageById = (messages, id) =>


nit: it'd be helpful to have comment with explanation and example

azasypkin · 2018-06-01T11:44:56Z

packages/kbn-i18n/src/core.js

+
+        formattedMessage = msg.format(values);
+      } catch (e) {
+        showError(


question: I'm wondering if we shouldn't swallow error when we had message for the specified locale, but failed to format it. Looks like it's a way too tolerant to developer/translator mistakes. What do you think?

azasypkin · 2018-06-01T11:46:34Z

packages/kbn-i18n/src/core.js

+          this.getFormats()
+        );
+
+        formattedMessage = msg.format(values);


question: can't we just return here? Or there are cases when format doesn't throw and return falsy value?

elasticmachine · 2018-06-05T15:32:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-06-06T09:52:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

rhoboat · 2018-06-07T13:54:38Z

packages/kbn-i18n/src/index.js

+export * from './react_i18n';
+
+export { default as I18n } from './core';
+export { default as i18n } from './i18n';


This file has some issues.

I don't see the point of the file src/i18n.js as it currently stands. I agree with Aleh about renaming src/core.js to src/i18n.js, and then we can export the I18n class.

what is the point of the unused import on line 20?

looking at how the docs read, people are only going to use the I18n class? What is the point of the i18n class? is it just to provide a default instance of the class? Is that used anywhere in this library, or could it just be created by consumers of the library?

I would like to hear the intention behind this current approach, if you feel my comments are in error!

Ah, I see the i18n instance is what's used to create the angular and react providers. Let me give my last point (above) a little more thought.

rhoboat · 2018-06-07T14:17:07Z

packages/kbn-i18n/src/core.js

+    return this.defaultLocale;
+  }
+
+  defineFormats(formats) {


setFormats ?

elasticmachine · 2018-06-07T14:24:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

rhoboat · 2018-06-07T14:40:13Z

packages/kbn-i18n/README.md


 ```js
-import i18n from 'kbn-i18n';
+import { I18n } from '@kbn/i18n/src/i18n';


You can actually say import I18n from @kbn/i18n; because inside of packages/kbn-i18n/package.json you have a line "main": "src/index.js".

rhoboat · 2018-06-07T14:41:12Z

src/ui/ui_render/ui_render_mixin.js

 import { props, reduce as reduceAsync } from 'bluebird';
 import Boom from 'boom';
 import { resolve } from 'path';
+import { I18n } from '@kbn/i18n/src/i18n';


import { I18n } from '@kbn/i18n';

maksim-tolo · 2018-06-07T14:42:29Z

#19728, #19731, #19732

rhoboat · 2018-06-07T14:44:50Z

packages/kbn-i18n/src/angular/provider.js

+import { i18n } from '../i18n';
+
+export function i18nProvider() {
+  this.addMessages = function(messages, locale) {


[redacted]
I got some clarity from Aleh about this. We always want the inner i18n context for this. Hm. I'm still thinking if there's another way. That's a bit like an antipattern.

Okay, I have some thoughts now that I understand what we're doing.
Why I think there is something weird/antipattern/code smell:

Normally when we use JavaScript class syntax, if the class's methods use this internally, we design it that way so that any instance of that class can pass along its own this context, and the methods use that context. However in this case, we always want to use the i18n instance's this.

The angular provider (i18nProvider) is not an instance or a class, yet it is setting its own this.[properties]. That's a strange thing. Is it because of how angular expects the provider? Forgive me I don't remember exactly how angular 1.x works and maybe this is just something we have to do.

Additionally, the capital-case I18n is used as a singleton (as i18n), so the this is always going to be the same, right?

And also, arrow functions are normally good for solving issues of the wrong this context, but using them here would be absolutely wrong. It would pass along the wrong this. That seems to me like we're creating unnecessary indirection within i18n.

It’s kind of weird to have to do i18n.translate.bind(i18n) because it seems like calling translate on the i18n should already provide the right this context. We’re calling it on the i18n object, after all.

All this points to how I18n is designed. Would it work to forgo the class, make all the methods no longer call this.[whatever], and make I18n a function that accepts messages/locale and return an object that do its methods on that? It would follow that the methods of I18n would not have to explicitly call this.[whatever] because there's no need to take the this context--it's always the same context. And thus the angular provider would be something more like a simple wrapper around I18n. Basically, if you want to talk about this in the classical way, this would become a fully static class. Does that make sense, and would it make sense for this library?

Something like this?

export function I18n(messages, ...) { const _messages = // set it const _currentLocale = // set it return { getMessages: function() { return _messages[_currentLocale]; }, // ... }; }

There would be less indirection, less potential confusion around this because it isn't actually being used as intended, and I think the angular provider becomes simpler, too. What do you think?

I removed I18n class and made i18n a singleton because I don't see real use cases for multiple I18n instances. So we no longer use this keyword in i18n engine.

rhoboat · 2018-06-07T15:29:15Z

packages/kbn-i18n/src/server/__tests__/i18n.js

+ */
+
+import expect from 'expect.js';
+import _ from 'lodash';


maybe just import { isEqual } from 'lodash'; ?

Hmm, I'm wondering why built-in Jest/Mocha/Expect matchers didn't do the job and we resorted to Lodash?

elasticmachine · 2018-06-26T16:58:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

rhoboat

LGTM, with a request for unit tests within the kbn-i18n lib. I'm assuming that's meant to be part of this work, but please comment if it's not.

rhoboat · 2018-06-26T17:48:30Z

packages/kbn-i18n/src/core/i18n.js

+import { formats as EN_FORMATS } from './formats';
+
+// Add all locale data to `IntlMessageFormat`.
+// TODO: Use dynamic import for asynchronous loading of specific locale data


Before merging this in, I think we should either resolve the TODO comments or else create an issue and drop the link here, so we don't lose track.

+1 for issue

rhoboat · 2018-06-26T17:48:39Z

tasks/verify_translations.js

@@ -17,11 +17,13 @@
 * under the License.
 */

+// TODO: Integrate a new tool for translations checking


Before merging this in, I think we should either resolve the TODO comments or else create an issue and drop the link here, so we don't lose track.

rhoboat

LGTM provided we do something about the TODO comments (left notes for those).

azasypkin

LGTM once you merge master in, resolve existing minor conflicts and make CI happy.

elasticmachine · 2018-06-27T11:27:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

maksim-tolo · 2018-06-27T15:58:25Z

I found a small issue related to Intl object in node.js. By default Node.js is installed with small-icu flag, which means that Intl supports only English locale. In order to add full ICU data, we have to add full-icu module to the project dependencies and set NODE_ICU_DATA environment variable to ./node_modules/full-icu before starting node.js process. Here you can find more details https://nodejs.org/api/intl.html. It affects only server usage of i18n. We will fix this issue in a followup PR.

azasypkin · 2018-06-27T16:11:38Z

In order to add full ICU data, we have to add full-icu module to the project dependencies and set NODE_ICU_DATA environment variable to ./node_modules/full-icu before starting node.js process. Here you can find more details https://nodejs.org/api/intl.html. It affects only server usage of i18n. We will fix this issue in a followup PR.

Can you please file a separate issue (and link with this PR) where we can discuss/investigate alternatives and their impact on distributable size and the way Kibana is run?

* Add draft implementation of I18n engine * Add i18n loader * kbn-i18n refactoring * Fix react i18n context and update doc * i18n engine refactoring * Fix locales data loading and add more jsdoc comments * Fix verify_translations task * I18n tests refactoring * Add build scripts to kbn-i18n package * Fix some bugs * Move uiI18nMixin into ui_i18n folder * Add 'browser' field to kbn-i18n package.json * Get rid of "showError" method * Make i18n and i18nLoader a singleton object * Add default locale as fallback if translation files were not registered * Update yarn.lock * kbn-i18n fix * Add default formats * Try to fix build * Add more examples into kbn-i18n/README.md * kbn-i18n fix * Fix app_bootstrap tests * Add links to issues in TODO comments

LeanidShutau · 2018-08-01T11:26:53Z

6.x/6.5: 1d0e438

Add draft implementation of I18n engine

d569825

maksim-tolo added WIP Work in progress Project:i18n labels May 30, 2018

maksim-tolo requested review from epixa, spalger, azasypkin and rhoboat May 30, 2018 15:13

azasypkin requested changes Jun 1, 2018

View reviewed changes

azasypkin added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jun 1, 2018

Add i18n loader

e4ceebb

jinmu03 requested a review from Bargs June 5, 2018 06:17

kbn-i18n refactoring

be7e201

Fix react i18n context and update doc

a6eb6cd

maksim-tolo added 4 commits June 6, 2018 19:12

i18n engine refactoring

d21f850

Fix locales data loading and add more jsdoc comments

6ebf046

Fix verify_translations task

dc50b75

I18n tests refactoring

929c406

rhoboat reviewed Jun 7, 2018

View reviewed changes

packages/kbn-i18n/src/core.js Outdated

return this.defaultLocale;

}

defineFormats(formats) {

Copy link

rhoboat Jun 7, 2018

Choose a reason for hiding this comment

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

setFormats ?

rhoboat reviewed Jun 7, 2018

View reviewed changes

Add build scripts to kbn-i18n package

1deea3a

maksim-tolo added 2 commits June 26, 2018 17:55

kbn-i18n fix

b017a83

Fix app_bootstrap tests

cb9f42e

rhoboat suggested changes Jun 26, 2018

View reviewed changes

rhoboat reviewed Jun 26, 2018

View reviewed changes

rhoboat approved these changes Jun 26, 2018

View reviewed changes

azasypkin approved these changes Jun 27, 2018

View reviewed changes

maksim-tolo added 2 commits June 27, 2018 13:10

Add links to issues in TODO comments

1e2043a

Merge remote-tracking branch 'upstream/master' into i18n-engine

b01a813

maksim-tolo mentioned this pull request Jun 28, 2018

[I18n] Integrate full-icu module #20288

Closed

maksim-tolo merged commit f522b31 into elastic:master Jun 28, 2018

maksim-tolo deleted the i18n-engine branch June 28, 2018 09:39

LeanidShutau mentioned this pull request Jul 31, 2018

[6.x] Add draft implementation of I18n engine (#19555) #21473

Merged

LeanidShutau added backported v6.5.0 and removed review labels Aug 1, 2018

This was referenced Aug 24, 2018

[I18n] Implement Angular i18n wrapper #19731

Closed

[I18n] Implement I18n engine #19732

Closed

[I18n] Add date/time formats into json files #19896

Closed

		@@ -107,6 +109,7 @@ when missing translations
		- `getDefaultLocale()` - returns the default locale
		- `defineFormats(formats: object)` - supplies a set of options to the underlying formatter.


		const hasValues = values => Object.keys(values).length > 0;

		const addLocaleData = localeData => {


		const message = getMessageById(this.getMessages(), id);

		if (!hasValues(values) && process.env.NODE_ENV === 'production') {


		const EN_LOCALE = 'en';

		const getMessageById = (messages, id) =>

Add draft implementation of I18n engine #19555

Add draft implementation of I18n engine #19555

Conversation

maksim-tolo commented May 30, 2018 • edited Loading

elasticmachine commented May 30, 2018

💔 Build Failed

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

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 Jun 5, 2018

💔 Build Failed

elasticmachine commented Jun 6, 2018

💔 Build Failed

rhoboat Jun 7, 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 Jun 7, 2018

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksim-tolo commented Jun 7, 2018

rhoboat Jun 7, 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

elasticmachine commented Jun 26, 2018

💚 Build Succeeded

rhoboat 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

rhoboat left a comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

elasticmachine commented Jun 27, 2018

💚 Build Succeeded

maksim-tolo commented Jun 27, 2018

azasypkin commented Jun 27, 2018

LeanidShutau commented Aug 1, 2018

maksim-tolo commented May 30, 2018 •

edited

Loading

rhoboat Jun 7, 2018 •

edited

Loading

rhoboat Jun 7, 2018 •

edited

Loading