Update with review comments

From review https://github.com/elastic/kibana/pull/7545#issuecomment-231147435
following comments updated:
- 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)
- 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.

src/plugins/i18n/.eslintrc

@@ -1,2 +0,0 @@
----
-extends: "@elastic/kibana"

src/plugins/i18n/.gitignore

@@ -1,2 +0,0 @@
-npm-debug.log*
-node_modules

src/plugins/i18n/README.md

@@ -1,11 +0,0 @@
-# i18n
-
-> Core plugin which manages Kibana globalization
-
----
-
-The main goal is to manage translations for all views in Kibana. 
-
-The i18n plugin provides an API for plugins to register translations. This API takes top level directory of where the plugin translations reside and then concatenates thes translations into a translation bundle per language. The translation files need to be a valid JSON object of key, value elements. The IDs need to be unique so it is recommended to use plugin name as a prefix.
-
-The i18n plugin provides an additional API to get registered translations for a language.  This return a JSON object of all the translations elements iregistered for that language.

src/plugins/i18n/package.json

@@ -1,25 +1,4 @@
 {
   "name": "i18n",
-  "version": "0.0.0",
-  "description": "Core plugin which manages Kibana globalization",
-  "main": "index.js",
-  "scripts": {
-    "lint": "eslint",
-    "start": "plugin-helpers start",
-    "test:server": "plugin-helpers test_server",
-    "test:browser": "plugin-helpers test_browser",
-    "build": "plugin-helpers build",
-    "postinstall": "plugin-helpers postinstall"
-  },
-  "devDependencies": {
-    "@elastic/eslint-config-kibana": "0.0.2",
-    "@elastic/plugin-helpers": "5.0.0-beta1",
-    "accept-language-parser": "^1.1.2",
-    "async": "^2.0.0-rc.6",
-    "babel-eslint": "4.1.8",
-    "bluebird": "^3.4.0",
-    "boom": "^3.2.1",
-    "eslint": "1.10.3",
-    "eslint-plugin-mocha": "1.1.0"
-  }
+  "version": "1.0.0"
 }

src/plugins/i18n/server/__tests__/i18n.js

@@ -1,15 +1,16 @@
 import expect from 'expect.js';
+import process from 'child_process';
+import i18n from '../i18n/i18n';
 
-var process = require('child_process');
-var i18n = require('../i18n');
+const DATA_PATH = __dirname + '/fixtures';
 
 describe('Test plugin translations details for test_plugin_1', function () {
-  var pluginName = 'test_plugin_1';
-  var pluginTranslationPath = __dirname + '/data/translations/' + pluginName;
+  const pluginName = 'test_plugin_1';
+  const pluginTranslationPath = DATA_PATH + '/translations/' + pluginName;
 
   it('Translation languages exist', function (done) {
-    var result = true;
-    var expectedLanguages = ['en', 'de'];
+    let result = true;
+    const expectedLanguages = ['en', 'de'];
     getPluginTranslationLanguages(pluginName, pluginTranslationPath, function (err, actualLanguages) {
       if (err) {
         console.log(err);
@@ -18,7 +19,7 @@ describe('Test plugin translations details for test_plugin_1', function () {
         if (actualLanguages.length !== expectedLanguages.length) {
           result = false;
         } else {
-          var index = actualLanguages.length;
+          let index = actualLanguages.length;
           actualLanguages.sort();
           expectedLanguages.sort();
           while (index--) {
@@ -35,8 +36,8 @@ describe('Test plugin translations details for test_plugin_1', function () {
   });
 
   it('Translation files exist', function (done) {
-    var result = true;
-    var expectedFiles = [
+    let result = true;
+    const expectedFiles = [
       pluginTranslationPath + '/view1/de.json',
       pluginTranslationPath + '/view1/en.json',
       pluginTranslationPath + '/view2/en.json'
@@ -49,7 +50,7 @@ describe('Test plugin translations details for test_plugin_1', function () {
         if (actualFiles.length !== expectedFiles.length) {
           result = false;
         } else {
-          var index = actualFiles.length;
+          let index = actualFiles.length;
           actualFiles.sort();
           expectedFiles.sort();
           while (index--) {
@@ -67,11 +68,11 @@ describe('Test plugin translations details for test_plugin_1', function () {
 });
 
 describe('Test registering translations for test_plugin_1', function () {
-  var pluginName = 'test_plugin_1';
-  var pluginTranslationPath = __dirname + '/data/translations/' + pluginName;
+  const pluginName = 'test_plugin_1';
+  const pluginTranslationPath = DATA_PATH + '/translations/' + pluginName;
 
   it('Register translations' , function (done) {
-    var result = true;
+    let result = true;
     i18n.registerTranslations(pluginTranslationPath, function (err) {
       if (err) {
         console.log(err);
@@ -83,10 +84,10 @@ describe('Test registering translations for test_plugin_1', function () {
   });
 
   it('EN translations are registered' , function (done) {
-    var result = true;
-    var language = 'en';
-    var expectedTranslationJsonFile = __dirname + '/data/reference/' + pluginName + '/' + language + '.json';
-    var expectedTranslationJson = require(expectedTranslationJsonFile);
+    let result = true;
+    const language = 'en';
+    const expectedTranslationJsonFile = DATA_PATH + '/reference/' + pluginName + '/' + language + '.json';
+    const expectedTranslationJson = require(expectedTranslationJsonFile);
 
     i18n.getRegisteredLanguageTranslations(language, function (err, actualTranslationJson) {
       if (err) {
@@ -103,10 +104,10 @@ describe('Test registering translations for test_plugin_1', function () {
   });
 
   it('DE translations are registered' , function (done) {
-    var result = true;
-    var language = 'de';
-    var expectedTranslationJsonFile = __dirname + '/data/reference/' + pluginName + '/' + language + '.json';
-    var expectedTranslationJson = require(expectedTranslationJsonFile);
+    let result = true;
+    const language = 'de';
+    const expectedTranslationJsonFile = DATA_PATH + '/reference/' + pluginName + '/' + language + '.json';
+    const expectedTranslationJson = require(expectedTranslationJsonFile);
 
     i18n.getRegisteredLanguageTranslations(language, function (err, actualTranslationJson) {
       if (err) {
@@ -123,8 +124,8 @@ describe('Test registering translations for test_plugin_1', function () {
   });
 
   it('Translation languages are registered', function (done) {
-    var expectedLanguages = ['en', 'de'];
-    var result = true;
+    const expectedLanguages = ['en', 'de'];
+    let result = true;
 
     i18n.getRegisteredTranslationLanguages(function (err, actualLanguages) {
       if (err) {
@@ -135,7 +136,7 @@ describe('Test registering translations for test_plugin_1', function () {
       if (actualLanguages.length !== expectedLanguages.length) {
         result = false;
       } else {
-        var index = actualLanguages.length;
+        let index = actualLanguages.length;
         actualLanguages.sort();
         expectedLanguages.sort();
         while (index--) {
@@ -151,7 +152,7 @@ describe('Test registering translations for test_plugin_1', function () {
   });
 
   after(function (done) {
-    var translationStorePath = i18n.getTranslationStoragePath();
+    const translationStorePath = i18n.getTranslationStoragePath();
     process.execSync('rm -rf ' + translationStorePath);
     done();
   });
@@ -159,9 +160,9 @@ describe('Test registering translations for test_plugin_1', function () {
 
 describe('Test registering translations for test_plugin_1 and test_plugin_2', function () {
   it('Register translations for test_plugin_1' , function (done) {
-    var result = true;
-    var pluginName = 'test_plugin_1';
-    var pluginTranslationPath = __dirname + '/data/translations/' + pluginName;
+    let result = true;
+    const pluginName = 'test_plugin_1';
+    const pluginTranslationPath = DATA_PATH + '/translations/' + pluginName;
     i18n.registerTranslations(pluginTranslationPath, function (err) {
       if (err) {
         console.log(err);
@@ -173,9 +174,9 @@ describe('Test registering translations for test_plugin_1 and test_plugin_2', fu
   });
 
   it('Register translations for test_plugin_2' , function (done) {
-    var result = true;
-    var pluginName = 'test_plugin_2';
-    var pluginTranslationPath = __dirname + '/data/translations/' + pluginName;
+    let result = true;
+    const pluginName = 'test_plugin_2';
+    const pluginTranslationPath = DATA_PATH + '/translations/' + pluginName;
     i18n.registerTranslations(pluginTranslationPath, function (err) {
       if (err) {
         console.log(err);
@@ -187,10 +188,10 @@ describe('Test registering translations for test_plugin_1 and test_plugin_2', fu
   });
 
   it('EN translations are registered' , function (done) {
-    var result = true;
-    var language = 'en';
-    var expectedTranslationJsonFile = __dirname + '/data/reference/' + language + '.json';
-    var expectedTranslationJson = require(expectedTranslationJsonFile);
+    let result = true;
+    const language = 'en';
+    const expectedTranslationJsonFile = DATA_PATH + '/reference/' + language + '.json';
+    const expectedTranslationJson = require(expectedTranslationJsonFile);
 
     i18n.getRegisteredLanguageTranslations(language, function (err, actualTranslationJson) {
       if (err) {
@@ -207,10 +208,10 @@ describe('Test registering translations for test_plugin_1 and test_plugin_2', fu
   });
 
   it('DE translations are registered' , function (done) {
-    var result = true;
-    var language = 'de';
-    var expectedTranslationJsonFile = __dirname + '/data/reference/' + language + '.json';
-    var expectedTranslationJson = require(expectedTranslationJsonFile);
+    let result = true;
+    const language = 'de';
+    const expectedTranslationJsonFile = DATA_PATH + '/reference/' + language + '.json';
+    const expectedTranslationJson = require(expectedTranslationJsonFile);
 
     i18n.getRegisteredLanguageTranslations(language, function (err, actualTranslationJson) {
       if (err) {
@@ -227,8 +228,8 @@ describe('Test registering translations for test_plugin_1 and test_plugin_2', fu
   });
 
   it('Translation languages are registered', function (done) {
-    var expectedLanguages = ['en', 'de'];
-    var result = true;
+    const expectedLanguages = ['en', 'de'];
+    let result = true;
 
     i18n.getRegisteredTranslationLanguages(function (err, actualLanguages) {
       if (err) {
@@ -239,7 +240,7 @@ describe('Test registering translations for test_plugin_1 and test_plugin_2', fu
       if (actualLanguages.length !== expectedLanguages.length) {
         result = false;
       } else {
-        var index = actualLanguages.length;
+        let index = actualLanguages.length;
         actualLanguages.sort();
         expectedLanguages.sort();
         while (index--) {
@@ -255,15 +256,15 @@ describe('Test registering translations for test_plugin_1 and test_plugin_2', fu
   });
 
   after(function (done) {
-    var translationStorePath = i18n.getTranslationStoragePath();
+    const translationStorePath = i18n.getTranslationStoragePath();
     process.execSync('rm -rf ' + translationStorePath);
     done();
   });
 });
 
 function getPluginTranslationLanguages(pluginName, pluginTranslationPath, cb) {
-  var translationFiles = [];
-  var languageList = [];
+  let translationFiles = [];
+  let languageList = [];
   i18n.getPluginTranslationDetails(pluginTranslationPath, translationFiles, languageList, function (err) {
     if (err) return cb(err);
     return cb(null, languageList);
@@ -271,8 +272,8 @@ function getPluginTranslationLanguages(pluginName, pluginTranslationPath, cb) {
 }
 
 function getPluginTranslationFiles(pluginName, pluginTranslationPath, cb) {
-  var translationFiles = [];
-  var languageList = [];
+  let translationFiles = [];
+  let languageList = [];
   i18n.getPluginTranslationDetails(pluginTranslationPath, translationFiles, languageList, function (err) {
     if (err) return cb(err);
     return cb(null, translationFiles);
@@ -280,9 +281,9 @@ function getPluginTranslationFiles(pluginName, pluginTranslationPath, cb) {
 }
 
 function compareTranslations(actual, expected) {
-  var equal = true;
+  let equal = true;
 
-  for (var key in expected) {
+  for (let key in expected) {
     if (!actual.hasOwnProperty(key)) {
       equal = false;
       break;

src/plugins/i18n/server/i18n/i18n.js

@@ -1,15 +1,14 @@
-var async = require('async');
-var fs = require('fs');
-var kibanaPackage = require('../../../utils/package_json');
-var mkdirp = require('mkdirp');
-var os = require('os');
-var path = require('path');
-var process = require('child_process');
+import fs from 'fs';
+import kibanaPackage from '../../../../utils/package_json';
+import mkdirp from 'mkdirp';
+import os from 'os';
+import path from 'path';
+import process from 'child_process';
 
 const TRANSLATION_FILE_EXTENSION = 'json';
-const TRANSLATION_STORE_PATH = kibanaPackage.__dirname + '/data/translations';
+const TRANSLATION_STORE_PATH = kibanaPackage.__dirname + '/fixtures/translations';
 
-var getPluginTranslationDetails = function (pluginTranslationPath, translationFiles, languageList, callback) {
+let getPluginTranslationDetails = function (pluginTranslationPath, translationFiles, languageList, callback) {
   try {
     getFilesRecursivelyFromTopDir(pluginTranslationPath, translationFiles, languageList);
   } catch (err) {
@@ -19,14 +18,14 @@ var getPluginTranslationDetails = function (pluginTranslationPath, translationFi
 };
 
 //TODO(hickeyma): Update to use https://github.com/elastic/kibana/pull/7562
-var getTranslationStoragePath = function () {
+let getTranslationStoragePath = function () {
   return TRANSLATION_STORE_PATH;
 };
 
-var getRegisteredTranslationLanguages = function (cb) {
-  var translationFiles = [];
-  var languageList = [];
-  var translationStorePath = getTranslationStoragePath();
+let getRegisteredTranslationLanguages = function (cb) {
+  let translationFiles = [];
+  let languageList = [];
+  const translationStorePath = getTranslationStoragePath();
   try {
     getTranslationDetailsFromDirectory(translationStorePath, translationFiles, languageList);
   } catch (err) {
@@ -35,10 +34,10 @@ var getRegisteredTranslationLanguages = function (cb) {
   return cb(null, languageList);
 };
 
-var registerTranslations = function (pluginTranslationPath, cb) {
-  var translationFiles = [];
-  var languageList = [];
-  var translationStorePath = getTranslationStoragePath();
+let registerTranslations = function (pluginTranslationPath, cb) {
+  let translationFiles = [];
+  let languageList = [];
+  const translationStorePath = getTranslationStoragePath();
 
   getPluginTranslationDetails(pluginTranslationPath, translationFiles, languageList, function (err) {
     if (err) return cb(err);
@@ -47,12 +46,12 @@ var registerTranslations = function (pluginTranslationPath, cb) {
       if (!fs.existsSync(translationStorePath)) {
         mkdirp.sync(translationStorePath);
       }
-      for (var fileIndx in translationFiles) {
+      for (let fileIndx in translationFiles) {
         if (!translationFiles.hasOwnProperty(fileIndx)) continue;
-        var translationFile = translationFiles[fileIndx];
-        var translationFileName = getFileName(translationFile);
-        var translationJson = require(translationFile);
-        var fileToWrite = translationStorePath + '/' + translationFileName;
+        const translationFile = translationFiles[fileIndx];
+        const translationFileName = getFileName(translationFile);
+        const translationJson = require(translationFile);
+        const fileToWrite = translationStorePath + '/' + translationFileName;
         saveTranslationToFile(fileToWrite, translationJson);
       }
     } catch (err) {
@@ -63,13 +62,13 @@ var registerTranslations = function (pluginTranslationPath, cb) {
   return cb(null);
 };
 
-var getRegisteredLanguageTranslations = function (language, callback) {
-  var translationStorePath = getTranslationStoragePath();
-  var translationFileName = language + '.' + TRANSLATION_FILE_EXTENSION;
-  var translationFile = translationStorePath + '/' + translationFileName;
+let getRegisteredLanguageTranslations = function (language, callback) {
+  const translationStorePath = getTranslationStoragePath();
+  const translationFileName = language + '.' + TRANSLATION_FILE_EXTENSION;
+  const translationFile = translationStorePath + '/' + translationFileName;
   fs.readFile(translationFile, function (err, translationStr) {
     if (err) return callback(err);
-    var translationJson = [];
+    let translationJson = [];
     try {
       translationJson = JSON.parse(translationStr);
     } catch (e) {
@@ -80,14 +79,14 @@ var getRegisteredLanguageTranslations = function (language, callback) {
 };
 
 function saveTranslationToFile(translationFullFileName, translationToAddJson) {
-  var jsonToWrite = [];
+  let jsonToWrite = [];
   if (fs.existsSync(translationFullFileName)) {
-    var currentTranslationJson = require(translationFullFileName);
+    const currentTranslationJson = require(translationFullFileName);
     jsonToWrite = currentTranslationJson;
-    for (var key in translationToAddJson) {
+    for (let key in translationToAddJson) {
       if (translationToAddJson.hasOwnProperty(key)) {
-        var attrName = key;
-        var attrValue = translationToAddJson[key];
+        const attrName = key;
+        const attrValue = translationToAddJson[key];
         jsonToWrite[attrName] = attrValue;
       }
     }
@@ -99,8 +98,8 @@ function saveTranslationToFile(translationFullFileName, translationToAddJson) {
 
 function getFilesRecursivelyFromTopDir(topDir, translationFiles, languageList) {
   fs.readdirSync(topDir).forEach(function (name) {
-    var fullPath = path.join(topDir, name);
-    var stat = fs.statSync(fullPath);
+    const fullPath = path.join(topDir, name);
+    const stat = fs.statSync(fullPath);
     if (stat.isDirectory()) {
       getTranslationDetailsFromDirectory(fullPath, translationFiles, languageList);
     }
@@ -108,16 +107,16 @@ function getFilesRecursivelyFromTopDir(topDir, translationFiles, languageList) {
 }
 
 function getTranslationDetailsFromDirectory(fullPath, translationFiles, languageList) {
-  var files = getFilesFromDir(fullPath);
-  var fileLength = files.length;
-  for (var i = 0; i < fileLength; i++) {
-    var fullFilePath = files[i];
-    var fileName = getFileName(fullFilePath);
-    var fileExt = fileName.split('.').pop();
+  const files = getFilesFromDir(fullPath);
+  const fileLength = files.length;
+  for (let i = 0; i < fileLength; i++) {
+    const fullFilePath = files[i];
+    const fileName = getFileName(fullFilePath);
+    const fileExt = fileName.split('.').pop();
     if (fileName === fileExt) continue;
     if (fileExt !== TRANSLATION_FILE_EXTENSION) continue;
     translationFiles.push(fullFilePath);
-    var lang  = fileName.substr(0, fileName.lastIndexOf('.'));
+    const lang  = fileName.substr(0, fileName.lastIndexOf('.'));
     if (languageList.indexOf(lang) !== -1) {
       continue;
     }
@@ -126,12 +125,12 @@ function getTranslationDetailsFromDirectory(fullPath, translationFiles, language
 }
 
 function getFilesFromDir(dir) {
-  var fileList = [];
+  let fileList = [];
 
-  var files = fs.readdirSync(dir);
-  for (var i in files) {
+  const files = fs.readdirSync(dir);
+  for (let i in files) {
     if (!files.hasOwnProperty(i)) continue;
-    var name = dir + '/' + files[i];
+    const name = dir + '/' + files[i];
     if (!fs.statSync(name).isDirectory()) {
       fileList.push(name);
     }

src/plugins/i18n/server/i18n/index.js

@@ -0,0 +1,17 @@
+import { i18n } from './i18n';
+
+let registerTranslations = function (pluginTranslationPath, cb) {
+  i18n.registerTranslations(pluginTranslationPath, cb);
+};
+
+let getRegisteredLanguageTranslations = function (language, cb) {
+  i18n.getRegisteredLanguageTranslations(language, cb);
+};
+
+let getRegisteredTranslationLanguages = function (cb) {
+  i18n.getRegisteredTranslationLanguages(cb);
+};
+
+module.exports.registerTranslations = registerTranslations;
+module.exports.getRegisteredLanguageTranslations = getRegisteredLanguageTranslations;
+module.exports.getRegisteredTranslationLanguages = getRegisteredTranslationLanguages;

src/plugins/i18n/server/route.js

@@ -1,6 +1,6 @@
 import Boom from 'boom';
 
-var i18n = require('./i18n');
+var i18n = require('./i18n/index');
 var langParser = require('accept-language-parser');
 
 const DEFAULT_LANGUAGE = 'en';