Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@Bargs, @epixa, @srl295, @shikhasriva: First draft of i18n core plugin.

@hickeyma I read through the code in the PR. Before I jump into feedback, let me make sure I understand everything correctly. I've written up some notes with my understanding of the API (both JS module and HTTP) thus far:

Module API

• registerPluginLanguageTranslations(pluginName, pluginTranslationPath, language) - registers a plugin's translation with provided pluginName, path to translation files, and language
• getRegisteredPluginLanguageTranslations(pluginName, language) - gets json translations based on plugin name and language
• getPluginTranslationDetails(pluginTranslationPath, translationFiles, languageList) - gets translationFiles and languageList from a path to translation files, updating the params passed in
• getRegisteredPluginStoragePath(pluginName) - returns path to temp storage of translation files for a given plugin name
• getRegisteredPluginLanguages(pluginName) - returns array of language codes for a given plugin name

I'm guessing registerPluginLanguageTranslations, getRegisteredPluginLanguageTranslations, getRegisteredPluginLanguages are the methods you expect to be most useful to client code?

HTTP API

/api/i18n/translations/{plugin} - returns JSON translations based on plugin name and accept header

I will add one general note. I'm worried about loading translations per plugin. A plugin might provide a very small piece of a page. For instance, the Visualize page which displays all the visualization types available could be displaying content from many different plugins. So if we're loading translations by plugin, it might take more than 10 round trips to the server to get all of the translations for that page. Unless we end up finding that these translation bundles are huge, I think it makes more sense to load an entire language, for all plugins, in one go.

@Bargs Thanks for the notes; good synopsis.

Module API

Good description of the APIs.

Additionally, registerPluginLanguageTranslations(pluginName, pluginTranslationPath, language) - bundles all translation files for language into the one translation file for language.

registerPluginLanguageTranslations and getRegisteredPluginLanguageTranslations are the most useful to client code.

HTTP API

This is good feedback that a plugin may not provide all code to a page or multiple pages. If this is the case then it makes sense to have an API which returns all translations bundled for all plugins. I will add this API.
Should we maintain the API to return translations per plugin as well?

Should we maintain the API to return translations per plugin as well?

Probably not, we can always add it if we realize we need it later.

Also, btw a PR (#7457) just got merged that gives plugins a dedicated data directory for writing to the filesystem. I think you'll find that helpful in this plugin.

I question the value of the HTTP API at all: can we not just have the kibana server grab translations for the current bundles on load and serve them on the initial request?

@epixa are you suggesting all language translations get built directly into each JS bundle?

No, I think we could serve the current language of the user as javascript in either the html or as a separate js file. In the same way that we serve up details about kibana itself in window.__KBN__.

I suppose inlining the translations (like __KBN__) would save a roundtrip, but now you have to worry about caching different versions of the page for different languages. That seems more like a nice optimization to me. Even if we had that, I think we'd still want a separate HTTP endpoint for serving translations when the user wants to switch languages, so it can be done dynamically without a page reload.

We need the translations when the contents are loading anyway, otherwise we can't translate things like the loading screen or the global error handler (not the angular notifier handler, the more global one). So at the very least, those translations will need to be served up with the HTML.

We shouldn't optimize for the experience of switching languages, and I'd prefer to outright eliminate any overhead in maintaining that experience by requiring a page reload. For the vast majority of installs, there will only be a single language configured. Most Kibana installs are private to a company, and most companies either have no use at all for supporting multiple languages or won't be interested in the ongoing burden of maintaining a multiple language install. There are exceptions, of course, but we should be optimizing for the most common usage.

(jumping in midstream…)
@epixa the purpose of the HTTP API is to facilitate loading of all of the current bundles. It's not for optimizing switching translations. The data pretty much has to go over HTTP anyway, so the point was that just as the kibana.bundle.js gets fetched in one GET, that one GET would also fetch any relevant translations. That GET would give the opportunity to do the content negotiation needed with the web browser.

But I think that language selection needs to happen during the initial load of the application one way or another, otherwise we can't translate the two screens I mentioned above. I agree that most of the translations can be loaded separately, but at least those translations need to be applied when the initial html is created on the server, so we're already going to need to do language determination at that time.

As for a separate request for translations, that really needs to happen during the initial load window (when you see the "one moment please" screen), which means that cannot be initiated from the client-side JS bundles themselves. It must either be embedded into the HTML (which is really only ideal for the very few strings that exist in the global context of the main html) or linked as a script tag along with the other bundles: https://github.com/elastic/kibana/blob/master/src/ui/views/ui_app.jade#L48-L53

or linked as a script tag along with the other bundles

That's what I thought would happen for initial content.

which means that cannot be initiated from the client-side JS bundles themselves

@epixa Yeah I see the miscommunication now. When I wrote "HTTP API" in my summary comment, I didn't mean "an API that can only be called via XHR". As I mentioned in this comment, the translations can be served by Hapi just like the static bundles are served by Hapi currently.

A couple more thoughts:

• If we want to build the language bundles at build time and/or plugin installation time, how will that work if the core i18n API is implemented as a plugin? Plugins don't get initialized until Kibana starts. I don't have the answer off the top of my head, I'd have to learn a bit more about the current plugin installation process. Something to think about.
• Should we divide language bundles per app? This is how the JS bundles currently get divided up. But this would require knowledge of which apps use which translations.
• Since using english strings as keys is a bad idea, how should we namespace these things? Plugin id probably? What checks should we run at compile time?

First draft of an example translation plugin structure: https://github.com/Bargs/management-es

At first glance I thought this might work at installation time without any modifications, but upon further inspection I've realized the plugin installer doesn't initialize the plugins, it only creates the js/css bundles. I think we should make it work at install time, so I need to give this some more thought.

So I think we have a couple options.

1. Just build the translation bundles on server startup.

   Pro: Requires no change to the current install/startup process. Kibana will heal itself if the translation bundles are deleted.

   Con: Adds additional work on server start up. Build/install time verifications are impossible. If we want to speed up server start up, I think we're going to run into some complexity with detecting changes and deciding when to invalidate the cache. We'll have to deal with additional complexity of deciding when to turn Kibana green, because it will need to wait for the async translation bundling to finish.

2. Create an additional plugin lifecycle hook specifically for install time tasks.

   Pro: Runs once at install time, could run some validations before ever booting Kibana. Other plugins may find this hook useful down the road.

   Con: No automatic healing without additional work. In order to lean on the existing plugin dependency graph, this would probably be enabled as an option on the minimal Kibana boot up already being used in the plugin installation process.

I'm leaning towards the second option. I think it would be pretty easy to implement and should avoid a lot of complexity at server startup time. What do you think @epixa?

@Bargs I like the example translation plugin structure.

I am interested to know more about the "additional plugin lifecycle hook" ! :-)

FYI: I have refactored the i18n plugin:

• To register translations with top translation directory only
• Parse a flattened translation JSON object to be key/value elements

Unless someone just outright hates the idea of adding an install-time hook to the server boot process that can be enabled in the plugin installer, I'm going to work on a prototype for it. It's a little weird adding it to the KbnServer constructor, but I think it's better than inventing a totally separate mechanism right now.

Here's a rough prototype of what I'm thinking: #7613

Can one of the admins verify this patch?

@hickeyma to follow up on your question from IRC, you'll want to replace my mock registration here with your actual i18n module.

My idea is that within the install hook, plugins can register helper functions (like registerTranslation) with the installContext, which other plugins can use in their own install hook functions. Essentially installContext is a simple dependency injection context for the install lifecycle that would allow plugins to develop implementations that rely on other plugins, without coupling them to the specific module paths in their dependencies. An added benefit is that we can lean on the existing dependency resolution code in the Kibana plugin system, so if one plugin relies on another, we just specify that as usual.

@Bargs I thought the mock registration provided some hooks back into the actual call to registerTranslations. Thanks for setting me straight. It worked as expected when I made the change as you suggested.

As promised, here's my first round of feedback. This probably looks like a massive list, but a lot of it is just coding convention stuff to make things more consistent with the rest of Kibana. This list might not be totally comprehensive either, but I wanted to get you some feedback asap so you're not blocked.

• README, .gitignore, and .eslintrc are not needed in a core plugin
• package.json only needs name and version
• unit tests need to go in a tests directory otherwise they won't get picked up by the grunt tasks. Also our convention is to name the test file with the same name as the module it's testing (so i18n_tests.js should just be i18n.js)
• Unit tests need to be updated based on new proposed plugin structure (single language file, not split by view)
• For consistency with the rest of the code base, rename the data directory to fixtures.
• Prefer const (or let if necessary). Don't use var.
• Use ES6 imports/exports rather than commonjs style
• Only export the i18n module's public API. For instance, I don't think getPluginTranslationDetails is used outside of the i18n module, so it shouldn't be exposed publicly. If you want to expose it for testing purposes, I would recommend creating an i18n directory with an index.js file that exports the module's public API, and a separate i18n.js file with the "private" API. index.js will be for public use, i18n.js will be for private internal use.
• All filesystem access should be async
• i18n module API should return promises for async operations instead of using callbacks. You can also probably clean up a lot of the internal i18n code by using http://bluebirdjs.com/docs/api/promise.promisify.html to wrap node functions that expect a callback

Thanks @Bargs for review comments. Updated and ready for review.

jenkins, test it

Code LGTM, let's see what Jenkins thinks!

jenkins, test this

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

Awesome and yes this is just the beginning... but a great start. Looking forward to next steps

Very exciting. Great job on this!

Can one of the admins verify this patch?

great stuff!

Great!