Repository: GoogleChrome/lighthouse
Pull request #9461 (2019-07-25 21:53:30)

i18n: align type names with docs
Follow up to #9114 
- renaming typedefs to align with CTC and LHL as defined in the [readme](https://github.com/GoogleChrome/lighthouse/blob/0cf90aa34708c539058e68c5692e838020419cd8/lighthouse-core/lib/i18n/README.md#terminology)
- fixing some placeholder `undefined` nonsense by tsc (https://github.com/GoogleChrome/lighthouse/pull/9114#discussion_r306576574)
- finishing up the suggestion from https://github.com/GoogleChrome/lighthouse/pull/8755#discussion_r280968989 and making sure all our instances of icu replacement values are typed as `Record<string, string|number>` since that's all we accept


Patch:
diff --git a/lighthouse-core/audits/dobetterweb/appcache-manifest.js b/lighthouse-core/audits/dobetterweb/appcache-manifest.js
index c525a7ed9..b20ab17af 100644
--- a/lighthouse-core/audits/dobetterweb/appcache-manifest.js
+++ b/lighthouse-core/audits/dobetterweb/appcache-manifest.js
@@ -49,13 +49,16 @@ class AppCacheManifestAttr extends Audit {
    * @return {LH.Audit.Product}
    */
   static audit(artifacts) {
-    const usingAppcache = artifacts.AppCacheManifest !== null;
-    const displayValue = usingAppcache ?
-      str_(UIStrings.displayValue, {AppCacheManifest: artifacts.AppCacheManifest}) : '';
+    // Fails if an AppCacheManifest was found.
+    if (artifacts.AppCacheManifest !== null) {
+      return {
+        score: 0,
+        displayValue: str_(UIStrings.displayValue, {AppCacheManifest: artifacts.AppCacheManifest}),
+      };
+    }
 
     return {
-      score: usingAppcache ? 0 : 1,
-      displayValue,
+      score: 1,
     };
   }
 }
diff --git a/lighthouse-core/audits/seo/canonical.js b/lighthouse-core/audits/seo/canonical.js
index fe3032bf8..463e6455a 100644
--- a/lighthouse-core/audits/seo/canonical.js
+++ b/lighthouse-core/audits/seo/canonical.js
@@ -178,7 +178,7 @@ class Canonical extends Audit {
     if (!URL.rootDomainsMatch(canonicalURL, baseURL)) {
       return {
         score: 0,
-        explanation: str_(UIStrings.explanationDifferentDomain, {url: canonicalURL}),
+        explanation: str_(UIStrings.explanationDifferentDomain, {url: canonicalURL.href}),
       };
     }
 
diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js
index 4ebd6c4fa..53c8dfd1d 100644
--- a/lighthouse-core/lib/i18n/i18n.js
+++ b/lighthouse-core/lib/i18n/i18n.js
@@ -127,7 +127,7 @@ function lookupLocale(locale) {
 
 /**
  * @param {string} icuMessage
- * @param {Record<string, *>} [values]
+ * @param {Record<string, string | number>} [values]
  */
 function _preprocessMessageValues(icuMessage, values = {}) {
   const clonedValues = JSON.parse(JSON.stringify(values));
@@ -166,7 +166,7 @@ function _preprocessMessageValues(icuMessage, values = {}) {
  * @typedef IcuMessageInstance
  * @prop {string} icuMessageId
  * @prop {string} icuMessage
- * @prop {*} [values]
+ * @prop {Record<string, string | number>|undefined} [values]
  */
 
 /** @type {Map<string, IcuMessageInstance[]>} */
@@ -178,7 +178,7 @@ const _ICUMsgNotFoundMsg = 'ICU message not found in destination locale';
  * @param {LH.Locale} locale
  * @param {string} icuMessageId
  * @param {string=} uiStringMessage The original string given in 'UIStrings', used as a backup if no locale message can be found
- * @param {*} [values]
+ * @param {Record<string, string | number>} [values]
  * @return {{formattedString: string, icuMessage: string}}
  */
 function _formatIcuMessage(locale, icuMessageId, uiStringMessage, values) {
@@ -266,7 +266,7 @@ function createMessageInstanceIdFn(filename, fileStrings) {
    * indexed id value in the form '{messageid} | # {index}'.
    *
    * @param {string} icuMessage
-   * @param {*} [values]
+   * @param {Record<string, string | number>} [values]
    * */
   const getMessageInstanceIdFn = (icuMessage, values) => {
     const keyname = Object.keys(mergedStrings).find(key => mergedStrings[key] === icuMessage);
@@ -317,7 +317,7 @@ function getFormatted(icuMessageIdOrRawString, locale) {
 /**
  * @param {LH.Locale} locale
  * @param {string} icuMessageId
- * @param {*} [values]
+ * @param {Record<string, string | number>} [values]
  * @return {string}
  */
 function getFormattedFromIdAndValues(locale, icuMessageId, values) {
diff --git a/lighthouse-core/lib/i18n/locales.js b/lighthouse-core/lib/i18n/locales.js
index 63ca94006..09a27caee 100644
--- a/lighthouse-core/lib/i18n/locales.js
+++ b/lighthouse-core/lib/i18n/locales.js
@@ -12,10 +12,10 @@
  * CLDR language aliases: https://www.unicode.org/cldr/charts/latest/supplemental/aliases.html
  */
 
-/** @typedef {Record<string, {message: string}>} LocaleMessages */
+/** @typedef {Record<string, {message: string}>} LhlMessages */
 
 // The keys within this const must exactly match the LH.Locale type in externs.d.ts
-/** @type {Record<LH.Locale, LocaleMessages>} */
+/** @type {Record<LH.Locale, LhlMessages>} */
 const locales = {
   'en-US': require('./locales/en-US.json'), // The 'source' strings, with descriptions
   'en': require('./locales/en-US.json'), // According to CLDR/ICU, 'en' == 'en-US' dates/numbers (Why?!)
diff --git a/lighthouse-core/lib/i18n/swap-locale.js b/lighthouse-core/lib/i18n/swap-locale.js
index 0624f6fca..702bf443e 100644
--- a/lighthouse-core/lib/i18n/swap-locale.js
+++ b/lighthouse-core/lib/i18n/swap-locale.js
@@ -50,7 +50,8 @@ function swapLocale(lhr, requestedLocale) {
 
   const locale = i18n.lookupLocale(requestedLocale);
   const {icuMessagePaths} = lhr.i18n;
-  const missingIcuMessageIds = /** @type {string[]} */([]);
+  /** @type {string[]} */
+  const missingIcuMessageIds = [];
 
   Object.entries(icuMessagePaths).forEach(([icuMessageId, messageInstancesInLHR]) => {
     for (const instance of messageInstancesInLHR) {
diff --git a/lighthouse-core/scripts/i18n/bake-ctc-to-lhl.js b/lighthouse-core/scripts/i18n/bake-ctc-to-lhl.js
index 435dec796..055082d48 100644
--- a/lighthouse-core/scripts/i18n/bake-ctc-to-lhl.js
+++ b/lighthouse-core/scripts/i18n/bake-ctc-to-lhl.js
@@ -14,19 +14,24 @@ const path = require('path');
 const LH_ROOT = path.join(__dirname, '../../../');
 
 /**
- * @typedef ICUMessageDefn
+ * @typedef CtcMessage
  * @property {string} message the message that is being translated
- * @property {string} [description] a string used by translators to give context to the message
- * @property {string} [meaning] an arbitrary strings used by translators to differentiate messages that have the same message
- * @property {Record<string, ICUPlaceholderDefn>} [placeholders] a set of values that are to be replaced in a message
+ * @property {string} description a string used by translators to give context to the message
+ * @property {string} [meaning] an arbitrary string used by translators to differentiate messages that have the same message
+ * @property {Record<string, CtcPlaceholder>|undefined} [placeholders] a set of values that are to be replaced in a message
  */
 
 /**
- * @typedef ICUPlaceholderDefn
+ * @typedef CtcPlaceholder
  * @property {string} content the string that will be substituted into a message
  * @property {string} [example] an example (to assist translators) of what the content may be in the final string
  */
 
+/**
+ * @typedef LhlMessage
+ * @property {string} message
+ */
+
 /**
  * Take a series of CTC format ICU messages and converts them to LHL format by
  * replacing $placeholders$ with their {ICU} values. Functional opposite of
@@ -57,11 +62,11 @@ const LH_ROOT = path.join(__dirname, '../../../');
  * Throws if there is a $placeholder$ in the message that has no corresponding
  * value in the placeholders object, or vice versa.
  *
- * @param {Record<string, ICUMessageDefn>} messages
- * @return {Record<string, {message: string}>}
+ * @param {Record<string, CtcMessage>} messages
+ * @return {Record<string, LhlMessage>}
  */
 function bakePlaceholders(messages) {
-  /** @type {Record<string, {message: string}>} */
+  /** @type {Record<string, LhlMessage>} */
   const bakedMessages = {};
 
   for (const [key, defn] of Object.entries(messages)) {
@@ -91,8 +96,11 @@ function bakePlaceholders(messages) {
 
 /**
  * @param {string} file
+ * @return {Record<string, CtcMessage>}
  */
 function loadCtcStrings(file) {
+  if (!file.endsWith('.ctc.json')) throw new Error('Can only load ctc files');
+
   const rawdata = fs.readFileSync(file, 'utf8');
   const messages = JSON.parse(rawdata);
   return messages;
@@ -100,7 +108,7 @@ function loadCtcStrings(file) {
 
 /**
  * @param {string} path
- * @param {Record<string, {message: string}>} localeStrings
+ * @param {Record<string, LhlMessage>} localeStrings
  */
 function saveLhlStrings(path, localeStrings) {
   fs.writeFileSync(path, JSON.stringify(localeStrings, null, 2) + '\n');
@@ -112,7 +120,7 @@ function saveLhlStrings(path, localeStrings) {
  * @return {Array<string>}
  */
 function collectAndBakeCtcStrings(dir, outputDir) {
-  const lhl = [];
+  const lhlFilenames = [];
   for (const filename of fs.readdirSync(dir)) {
     const fullPath = path.join(dir, filename);
     const relativePath = path.relative(LH_ROOT, fullPath);
@@ -123,10 +131,10 @@ function collectAndBakeCtcStrings(dir, outputDir) {
       const strings = bakePlaceholders(ctcStrings);
       const outputFile = outputDir + path.basename(filename).replace('.ctc', '');
       saveLhlStrings(outputFile, strings);
-      lhl.push(path.basename(filename));
+      lhlFilenames.push(path.basename(filename));
     }
   }
-  return lhl;
+  return lhlFilenames;
 }
 
 module.exports = {
diff --git a/lighthouse-core/scripts/i18n/collect-strings.js b/lighthouse-core/scripts/i18n/collect-strings.js
index bc6defa74..fedc27bb1 100644
--- a/lighthouse-core/scripts/i18n/collect-strings.js
+++ b/lighthouse-core/scripts/i18n/collect-strings.js
@@ -18,7 +18,8 @@ const collectAndBakeCtcStrings = require('./bake-ctc-to-lhl.js');
 const LH_ROOT = path.join(__dirname, '../../../');
 const UISTRINGS_REGEX = /UIStrings = (.|\s)*?\};\n/im;
 
-/** @typedef {import('./bake-ctc-to-lhl.js').ICUMessageDefn} ICUMessageDefn */
+/** @typedef {import('./bake-ctc-to-lhl.js').CtcMessage} CtcMessage */
+/** @typedef {Required<Pick<CtcMessage, 'message'|'placeholders'>>} IncrementalCtc */
 
 const ignoredPathComponents = [
   '**/.git/**',
@@ -61,6 +62,7 @@ function computeDescription(ast, property, value, startRange) {
       return {description, examples};
     }
 
+    /** @type {string} */
     const description = comment.value.replace('*', '').trim();
 
     // Make sure description is not empty
@@ -103,37 +105,33 @@ function computeDescription(ast, property, value, startRange) {
  *
  * @param {string} message
  * @param {Record<string, string>} examples
- * @return {ICUMessageDefn}
+ * @return {IncrementalCtc}
  */
 function convertMessageToCtc(message, examples = {}) {
-  const icuDefn = {
+  /** @type {IncrementalCtc} */
+  const ctc = {
     message,
     placeholders: {},
   };
 
   // Process each placeholder type
-  _processPlaceholderMarkdownCode(icuDefn);
+  _processPlaceholderMarkdownCode(ctc);
 
-  _processPlaceholderMarkdownLink(icuDefn);
+  _processPlaceholderMarkdownLink(ctc);
 
-  _processPlaceholderCustomFormattedIcu(icuDefn);
+  _processPlaceholderCustomFormattedIcu(ctc);
 
-  _processPlaceholderDirectIcu(icuDefn, examples);
+  _processPlaceholderDirectIcu(ctc, examples);
 
-  _ctcSanityChecks(icuDefn);
+  _ctcSanityChecks(ctc);
 
-  if (Object.entries(icuDefn.placeholders).length === 0) {
-    // @ts-ignore - if this is empty then force undefined so that it does not appear in the JSON
-    icuDefn.placeholders = undefined;
-  }
-
-  return icuDefn;
+  return ctc;
 }
 
 /**
  * Convert code spans into placeholders with examples.
  *
- * @param {ICUMessageDefn} icu
+ * @param {IncrementalCtc} icu
  */
 function _processPlaceholderMarkdownCode(icu) {
   // Check that number of backticks is even.
@@ -165,7 +163,7 @@ function _processPlaceholderMarkdownCode(icu) {
 /**
  * Convert markdown html links into placeholders.
  *
- * @param {ICUMessageDefn} icu
+ * @param {IncrementalCtc} icu
  */
 function _processPlaceholderMarkdownLink(icu) {
   // Check for markdown link common errors, ex:
@@ -215,7 +213,7 @@ function _processPlaceholderMarkdownLink(icu) {
  *    }
  *  }
  *
- * @param {ICUMessageDefn} icu
+ * @param {IncrementalCtc} icu
  */
 function _processPlaceholderCustomFormattedIcu(icu) {
   // Split on custom-formatted ICU: {var, number, type}
@@ -275,7 +273,7 @@ function _processPlaceholderCustomFormattedIcu(icu) {
 /**
  * Add examples for direct ICU replacement.
  *
- * @param {ICUMessageDefn} icu
+ * @param {IncrementalCtc} icu
  * @param {Record<string, string>} examples
  */
 function _processPlaceholderDirectIcu(icu, examples) {
@@ -312,7 +310,7 @@ function _processPlaceholderDirectIcu(icu, examples) {
  * Do some basic sanity checks to a ctc object to confirm that it is valid. Future
  * ctc regression catching should go here.
  *
- * @param {ICUMessageDefn} icu the ctc output message to verify
+ * @param {IncrementalCtc} icu the ctc output message to verify
  */
 function _ctcSanityChecks(icu) {
   // '$$' i.e. "Double Dollar" is always invalid in ctc.
@@ -327,14 +325,14 @@ function _ctcSanityChecks(icu) {
  * done while messages are in `ctc` format, and therefore modifies only the
  * messages themselves while leaving placeholders untouched.
  *
- * @param {Record<string, ICUMessageDefn>} messages
- * @return {Record<string, ICUMessageDefn>}
+ * @param {Record<string, CtcMessage>} messages
+ * @return {Record<string, CtcMessage>}
  */
 function createPsuedoLocaleStrings(messages) {
-  /** @type {Record<string, ICUMessageDefn>} */
+  /** @type {Record<string, CtcMessage>} */
   const psuedoLocalizedStrings = {};
-  for (const [key, defn] of Object.entries(messages)) {
-    const message = defn.message;
+  for (const [key, ctc] of Object.entries(messages)) {
+    const message = ctc.message;
     const psuedoLocalizedString = [];
     let braceCount = 0;
     let inPlaceholder = false;
@@ -370,7 +368,8 @@ function createPsuedoLocaleStrings(messages) {
     }
     psuedoLocalizedStrings[key] = {
       message: psuedoLocalizedString.join(''),
-      placeholders: defn.placeholders,
+      description: ctc.description,
+      placeholders: ctc.placeholders,
     };
   }
   return psuedoLocalizedStrings;
@@ -386,10 +385,10 @@ let collisions = 0;
  * Collects all LHL messsages defined in UIString from Javascript files in dir,
  * and converts them into CTC.
  * @param {string} dir absolute path
- * @return {Record<string, ICUMessageDefn>}
+ * @return {Record<string, CtcMessage>}
  */
 function collectAllStringsInDir(dir) {
-  /** @type {Record<string, ICUMessageDefn>} */
+  /** @type {Record<string, CtcMessage>} */
   const strings = {};
 
   const globPattern = path.join(path.relative(LH_ROOT, dir), '/**/*.js');
@@ -404,7 +403,7 @@ function collectAllStringsInDir(dir) {
     const content = fs.readFileSync(absolutePath, 'utf8');
     const exportVars = require(absolutePath);
     const regexMatches = UISTRINGS_REGEX.test(content);
-    const exportsUIStrings = Boolean(exportVars.UIStrings);
+    const exportsUIStrings = !!exportVars.UIStrings;
     if (!regexMatches && !exportsUIStrings) continue;
 
     if (regexMatches && !exportsUIStrings) {
@@ -431,19 +430,26 @@ function collectAllStringsInDir(dir) {
         const val = exportVars.UIStrings[key];
         const {description, examples} = computeDescription(ast, property, val, lastPropertyEndIndex);
         const converted = convertMessageToCtc(val, examples);
-        const messageKey = `${relativeToRootPath} | ${key}`;
 
-        /** @type {ICUMessageDefn} */
-        const icuDefn = {
+        // Don't include placeholders if there are none.
+        const placeholders = Object.keys(converted.placeholders).length === 0 ?
+            undefined :
+            converted.placeholders;
+
+        /** @type {CtcMessage} */
+        const ctc = {
           message: converted.message,
           description,
-          placeholders: converted.placeholders,
+          placeholders,
         };
 
+        const messageKey = `${relativeToRootPath} | ${key}`;
+        strings[messageKey] = ctc;
+
         // check for duplicates, if duplicate, add @description as @meaning to both
-        if (seenStrings.has(icuDefn.message)) {
-          icuDefn.meaning = icuDefn.description;
-          const seenId = seenStrings.get(icuDefn.message);
+        if (seenStrings.has(ctc.message)) {
+          ctc.meaning = ctc.description;
+          const seenId = seenStrings.get(ctc.message);
           if (seenId) {
             if (!strings[seenId].meaning) {
               strings[seenId].meaning = strings[seenId].description;
@@ -452,9 +458,8 @@ function collectAllStringsInDir(dir) {
             collisions++;
           }
         }
+        seenStrings.set(ctc.message, messageKey);
 
-        seenStrings.set(icuDefn.message, messageKey);
-        strings[messageKey] = icuDefn;
         lastPropertyEndIndex = property.range[1];
       }
     }
@@ -465,11 +470,11 @@ function collectAllStringsInDir(dir) {
 
 /**
  * @param {string} locale
- * @param {Record<string, ICUMessageDefn>} strings
+ * @param {Record<string, CtcMessage>} strings
  */
 function writeStringsToCtcFiles(locale, strings) {
   const fullPath = path.join(LH_ROOT, `lighthouse-core/lib/i18n/locales/${locale}.ctc.json`);
-  /** @type {Record<string, ICUMessageDefn>} */
+  /** @type {Record<string, CtcMessage>} */
   const output = {};
   const sortedEntries = Object.entries(strings).sort(([keyA], [keyB]) => keyA.localeCompare(keyB));
   for (const [key, defn] of sortedEntries) {


Test patch:
diff --git a/lighthouse-core/test/scripts/i18n/collect-strings-test.js b/lighthouse-core/test/scripts/i18n/collect-strings-test.js
index 575d3e21f..d1d2213a3 100644
--- a/lighthouse-core/test/scripts/i18n/collect-strings-test.js
+++ b/lighthouse-core/test/scripts/i18n/collect-strings-test.js
@@ -172,14 +172,14 @@ describe('Convert Message to Placeholder', () => {
   it('passthroughs a basic message unchanged', () => {
     const message = 'Hello World.';
     const res = collect.convertMessageToPlaceholders(message, undefined);
-    expect(res).toEqual({message, placeholders: undefined});
+    expect(res).toEqual({message, placeholders: {}});
   });
 
   it('passthroughs an ICU plural unchanged', () => {
     const message = '{var, select, male{Hello Mr. Programmer.} ' +
       'female{Hello Ms. Programmer} other{Hello Programmer.}}';
     const res = collect.convertMessageToPlaceholders(message, undefined);
-    expect(res).toEqual({message, placeholders: undefined});
+    expect(res).toEqual({message, placeholders: {}});
   });
 
   // TODO(exterkamp): more strict parsing for this case