Repository: GoogleChrome/lighthouse
Pull request #9687 (2019-09-17 19:16:18)

i18n: treat Infinity and NaN as numeric values
fixes the crasher part of #9684

The issue was that we were `JSON.parse(JSON.stringify(values))`ing to make sure we weren't modifying the original ICU replacement values, but both `Infinity` and `NaN` get converted to `null` when stringified, which then fails the check that we're only replacing numeric placeholders with numbers.

Instead, this copies over the values one by one to a new object, formatting if needed. Also smushes `_preformatValues` and `_processParsedElements` together because the first was mostly just calling the second, but the first had a way better name.


Patch:
diff --git a/.eslintrc.js b/.eslintrc.js
index f2483a6a6..6d28a329d 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -73,6 +73,14 @@ module.exports = {
     'valid-jsdoc': 0,
     'arrow-parens': 0,
   },
+  overrides: [
+    {
+      files: ['lighthouse-cli/test/smokehouse/**/*expectations.js'],
+      rules: {
+        'max-len': 0,
+      },
+    },
+  ],
   parserOptions: {
     ecmaVersion: 2018,
     ecmaFeatures: {
diff --git a/docs/recipes/lighthouse-plugin-example/package.json b/docs/recipes/lighthouse-plugin-example/package.json
index dd5ae3b83..a1cae9f45 100644
--- a/docs/recipes/lighthouse-plugin-example/package.json
+++ b/docs/recipes/lighthouse-plugin-example/package.json
@@ -3,6 +3,6 @@
   "private": true,
   "main": "./plugin.js",
   "peerDependencies": {
-    "lighthouse": "^4.2.0"
+    "lighthouse": "^5.0.0"
   }
 }
diff --git a/docs/recipes/lighthouse-plugin-example/readme.md b/docs/recipes/lighthouse-plugin-example/readme.md
index 51dcdb713..55b0c799b 100644
--- a/docs/recipes/lighthouse-plugin-example/readme.md
+++ b/docs/recipes/lighthouse-plugin-example/readme.md
@@ -7,8 +7,9 @@
  
 ## To run
 
-- Install as a (peer) dependency, parallel to `lighthouse`.
-- `npx lighthouse https://example.com --plugins=lighthouse-plugin-example --view`
+1. Install `lighthouse` v5 or later.
+2. Install the plugin as a (peer) dependency, parallel to `lighthouse`.
+3. Run `npx -p lighthouse lighthouse https://example.com --plugins=lighthouse-plugin-example --view`
 
 The input to `--plugins` will be loaded from `node_modules/`.
 
diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js
index e6eff94c3..e0e406482 100644
--- a/lighthouse-core/lib/i18n/i18n.js
+++ b/lighthouse-core/lib/i18n/i18n.js
@@ -135,21 +135,6 @@ function lookupLocale(locale) {
   return closestLocale || 'en';
 }
 
-/**
- * Preformat values for the message based on how they're used, like KB or milliseconds.
- * @param {string} icuMessage
- * @param {MessageFormat} messageFormatter
- * @param {Record<string, string | number>} [values]
- * @return {Record<string, string | number>}
- */
-function _preformatValues(icuMessage, messageFormatter, values = {}) {
-  const clonedValues = JSON.parse(JSON.stringify(values));
-
-  const elements = collectAllCustomElementsFromICU(messageFormatter.getAst().elements);
-
-  return _processParsedElements(icuMessage, Array.from(elements.values()), clonedValues);
-}
-
 /**
  * Function to retrieve all 'argumentElement's from an ICU message. An argumentElement
  * is an ICU element with an argument in it, like '{varName}' or '{varName, number, bytes}'. This
@@ -188,16 +173,21 @@ function collectAllCustomElementsFromICU(icuElements, seenElementsById = new Map
 }
 
 /**
- * This function takes a list of ICU argumentElements and a map of values and
- * will apply Lighthouse custom formatting to the values based on the argumentElement
- * format style.
- *
+ * Returns a copy of the `values` object, with the values formatted based on how
+ * they will be used in the `icuMessage`, e.g. KB or milliseconds. The original
+ * object is unchanged.
  * @param {string} icuMessage
- * @param {Array<ArgumentElement>} argumentElements
- * @param {Record<string, string | number>} [values]
+ * @param {MessageFormat} messageFormatter
+ * @param {Readonly<Record<string, string | number>>} values
  * @return {Record<string, string | number>}
  */
-function _processParsedElements(icuMessage, argumentElements, values = {}) {
+function _preformatValues(icuMessage, messageFormatter, values) {
+  const elementMap = collectAllCustomElementsFromICU(messageFormatter.getAst().elements);
+  const argumentElements = [...elementMap.values()];
+
+  /** @type {Record<string, string | number>} */
+  const formattedValues = {};
+
   for (const {id, format} of argumentElements) {
     // Throw an error if a message's value isn't provided
     if (id && (id in values) === false) {
@@ -205,10 +195,14 @@ function _processParsedElements(icuMessage, argumentElements, values = {}) {
         `that wasn't provided`);
     }
 
+    const value = values[id];
+
     // Direct `{id}` replacement and non-numeric values need no formatting.
-    if (!format || format.type !== 'numberFormat') continue;
+    if (!format || format.type !== 'numberFormat') {
+      formattedValues[id] = value;
+      continue;
+    }
 
-    const value = values[id];
     if (typeof value !== 'number') {
       throw new Error(`ICU Message "${icuMessage}" contains a numeric reference ("${id}") ` +
         'but provided value was not a number');
@@ -217,28 +211,34 @@ function _processParsedElements(icuMessage, argumentElements, values = {}) {
     // Format values for known styles.
     if (format.style === 'milliseconds') {
       // Round all milliseconds to the nearest 10.
-      values[id] = Math.round(value / 10) * 10;
+      formattedValues[id] = Math.round(value / 10) * 10;
     } else if (format.style === 'seconds' && id === 'timeInMs') {
       // Convert all seconds to the correct unit (currently only for `timeInMs`).
-      values[id] = Math.round(value / 100) / 10;
+      formattedValues[id] = Math.round(value / 100) / 10;
     } else if (format.style === 'bytes') {
       // Replace all the bytes with KB.
-      values[id] = value / 1024;
+      formattedValues[id] = value / 1024;
+    } else {
+      // For all other number styles, the value isn't changed.
+      formattedValues[id] = value;
     }
   }
 
   // Throw an error if a value is provided but has no placeholder in the message.
   for (const valueId of Object.keys(values)) {
-    // errorCode is a special case always allowed to help ease of LHError use.
-    if (valueId === 'errorCode') continue;
+    if (valueId in formattedValues) continue;
 
-    if (!argumentElements.find(el => el.id === valueId)) {
-      throw new Error(`Provided value "${valueId}" does not match any placeholder in ` +
-        `ICU message "${icuMessage}"`);
+    // errorCode is a special case always allowed to help LHError ease-of-use.
+    if (valueId === 'errorCode') {
+      formattedValues.errorCode = values.errorCode;
+      continue;
     }
+
+    throw new Error(`Provided value "${valueId}" does not match any placeholder in ` +
+      `ICU message "${icuMessage}"`);
   }
 
-  return values;
+  return formattedValues;
 }
 
 /**
@@ -260,7 +260,7 @@ const _ICUMsgNotFoundMsg = 'ICU message not found in destination locale';
  * @param {Record<string, string | number>} [values]
  * @return {{formattedString: string, icuMessage: string}}
  */
-function _formatIcuMessage(locale, icuMessageId, uiStringMessage, values) {
+function _formatIcuMessage(locale, icuMessageId, uiStringMessage, values = {}) {
   const localeMessages = LOCALES[locale];
   if (!localeMessages) throw new Error(`Unsupported locale '${locale}'`);
   let localeMessage = localeMessages[icuMessageId] && localeMessages[icuMessageId].message;


Test patch:
diff --git a/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html b/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
index 0f50e9022..32e759932 100644
--- a/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
+++ b/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
@@ -30,7 +30,7 @@
 <!-- Note: these will only fail when using the static-server.js, which supports the ?delay=true param.
      If you're using your own server, the resource will load instantly and the
      stylesheets will be ignored for being below the threshold. -->
-<link rel="stylesheet" href="./dobetterweb/dbw_tester.css?delay=100"> <!-- FAIL, when run under smokehouse -->
+<link rel="stylesheet" href="./dbw_tester.css?delay=100">
 <link rel="stylesheet" href="./unknown404.css?delay=200"> <!-- FAIL -->
 <link rel="stylesheet" href="./dbw_tester.css?delay=2200"> <!-- FAIL -->
 <link rel="stylesheet" href="./dbw_disabled.css?delay=200&isdisabled" disabled> <!-- PASS -->
diff --git a/lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js b/lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js
index 2a2597af7..c6a86c2d9 100644
--- a/lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js
+++ b/lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js
@@ -20,7 +20,7 @@ module.exports = [
         {
           tag: {
             tagName: 'LINK',
-            url: 'http://localhost:10200/dobetterweb/dobetterweb/dbw_tester.css?delay=100',
+            url: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=100',
           },
         },
         {
@@ -81,9 +81,38 @@ module.exports = [
         'errors-in-console': {
           score: 0,
           details: {
-            items: {
-              length: 8,
-            },
+            items: [
+              {
+                source: 'network',
+                description: 'Failed to load resource: the server responded with a status of 404 (Not Found)',
+                url: 'http://localhost:10200/dobetterweb/unknown404.css?delay=200',
+              },
+              {
+                source: 'other',
+                description: 'Application Cache Error event: Manifest fetch failed (404) http://localhost:10200/dobetterweb/clock.appcache',
+                url: 'http://localhost:10200/dobetterweb/dbw_tester.html',
+              },
+              {
+                source: 'network',
+                description: 'Failed to load resource: the server responded with a status of 404 (Not Found)',
+                url: 'http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000',
+              },
+              {
+                source: 'network',
+                description: 'Failed to load resource: the server responded with a status of 404 (Not Found)',
+                url: 'http://localhost:10200/favicon.ico',
+              },
+              {
+                source: 'network',
+                description: 'Failed to load resource: the server responded with a status of 404 (Not Found)',
+                url: 'http://localhost:10200/dobetterweb/unknown404.css?delay=200',
+              },
+              {
+                source: 'Runtime.exception',
+                description: 'Error: An error\n    at http://localhost:10200/dobetterweb/dbw_tester.html:57:38',
+                url: 'http://localhost:10200/dobetterweb/dbw_tester.html',
+              },
+            ],
           },
         },
         'is-on-https': {
@@ -143,7 +172,7 @@ module.exports = [
           details: {
             items: [
               {
-                url: 'http://localhost:10200/dobetterweb/dobetterweb/dbw_tester.css?delay=100',
+                url: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=100',
               },
               {
                 url: 'http://localhost:10200/dobetterweb/unknown404.css?delay=200',
diff --git a/lighthouse-core/test/lib/i18n/i18n-test.js b/lighthouse-core/test/lib/i18n/i18n-test.js
index f78ed3d9c..d4dcb7d25 100644
--- a/lighthouse-core/test/lib/i18n/i18n-test.js
+++ b/lighthouse-core/test/lib/i18n/i18n-test.js
@@ -81,7 +81,6 @@ describe('i18n', () => {
       expect(formattedStr).toEqual('happy test');
     });
 
-
     it('returns the formatted string with replacements', () => {
       const UIStrings = {testMessage: 'replacement test ({errorCode})'};
       const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
@@ -97,6 +96,25 @@ describe('i18n', () => {
       expect(_ => i18n.getFormatted(str_(UIStrings.testMessage), 'still-not-a-locale'))
         .toThrow(`Unsupported locale 'still-not-a-locale'`);
     });
+
+    it('does not alter the passed-in replacement values object', () => {
+      const UIStrings = {
+        testMessage: 'needs {count, number, bytes}KB test {str} in {timeInMs, number, seconds}s',
+      };
+      const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
+
+      const replacements = {
+        count: 2555,
+        str: '*units*',
+        timeInMs: 314159265,
+      };
+      const replacementsClone = JSON.parse(JSON.stringify(replacements));
+
+      const formattedStr = i18n.getFormatted(str_(UIStrings.testMessage, replacements), 'en');
+      expect(formattedStr).toEqual('needs 2KB test *units* in 314,159.3s');
+
+      expect(replacements).toEqual(replacementsClone);
+    });
   });
 
   describe('#lookupLocale', () => {
@@ -265,5 +283,13 @@ describe('i18n', () => {
         // eslint-disable-next-line max-len
         .toThrow(`Provided value "sirNotAppearingInThisString" does not match any placeholder in ICU message "Hello {timeInMs, number, seconds} World"`);
     });
+
+    it('formats correctly with NaN and Infinity numeric values', () => {
+      const helloInfinityStr = str_(UIStrings.helloBytesWorld, {in: Infinity});
+      expect(helloInfinityStr).toBeDisplayString('Hello ∞ World');
+
+      const helloNaNStr = str_(UIStrings.helloBytesWorld, {in: NaN});
+      expect(helloNaNStr).toBeDisplayString('Hello NaN World');
+    });
   });
 });