core(i18n): fix path bug resulting in invalid i18n id via npx (#14314)

connorjclark · web-flow · commit b8bcfee749b4 · 2022-12-12T15:05:57.000-08:00
diff --git a/core/lib/i18n/i18n.js b/core/lib/i18n/i18n.js
@@ -173,6 +173,11 @@ function lookupLocale(locales, possibleLocales) {
 function createIcuMessageFn(filename, fileStrings) {
   if (filename.startsWith('file://')) filename = url.fileURLToPath(filename);
 
+  // In the common case, `filename` is an absolute path that needs to be transformed
+  // to be relative to LH_ROOT. In other cases, `filename` might be the exact i18n identifier
+  // already (see: stack-packs.js, or bundled lighthouse).
+  if (path.isAbsolute(filename)) filename = path.relative(LH_ROOT, filename);
+
   /**
    * Combined so fn can access both caller's strings and i18n.UIStrings shared across LH.
    * @type {Record<string, string>}
@@ -189,8 +194,12 @@ function createIcuMessageFn(filename, fileStrings) {
     const keyname = Object.keys(mergedStrings).find(key => mergedStrings[key] === message);
     if (!keyname) throw new Error(`Could not locate: ${message}`);
 
-    const filenameToLookup = keyname in fileStrings ? filename : getModulePath(import.meta);
-    const unixStyleFilename = path.relative(LH_ROOT, filenameToLookup).replace(/\\/g, '/');
+    // `message` can be a UIString defined within the provided `fileStrings`, or it could be
+    // one of the common strings found in `i18n.UIStrings`.
+    const filenameToLookup = keyname in fileStrings ?
+      filename :
+      path.relative(LH_ROOT, getModulePath(import.meta));
+    const unixStyleFilename = filenameToLookup.replace(/\\/g, '/');
     const i18nId = `${unixStyleFilename} | ${keyname}`;
 
     return {
diff --git a/shared/test/localization/format-test.js b/shared/test/localization/format-test.js
@@ -11,6 +11,7 @@ import * as constants from '../../../core/config/constants.js';
 import * as format from '../../localization/format.js';
 import {locales} from '../../localization/locales.js';
 import {getModuleDirectory, getModulePath} from '../../../esm-utils.js';
+import {LH_ROOT} from '../../../root.js';
 
 const moduleDir = getModuleDirectory(import.meta);
 const modulePath = getModulePath(import.meta);
@@ -185,37 +186,41 @@ describe('format', () => {
       expect(formattedStr).toEqual('en-XZ cuerda!');
     });
 
-    it('overwrites existing locale strings', async () => {
-      const filename = 'core/audits/is-on-https.js';
-      const {UIStrings} = await import('../../../core/audits/is-on-https.js');
-      const str_ = i18n.createIcuMessageFn(filename, UIStrings);
-
-      // To start with, we get back the intended string..
-      const origTitle = format.getFormatted(str_(UIStrings.title), 'es-419');
-      expect(origTitle).toEqual('Usa HTTPS');
-      const origFailureTitle = format.getFormatted(str_(UIStrings.failureTitle), 'es-419');
-      expect(origFailureTitle).toEqual('No usa HTTPS');
-
-      // Now we declare and register the new string...
-      const localeData = {
-        'core/audits/is-on-https.js | title': {
-          'message': 'new string for es-419 uses https!',
-        },
-      };
-      format.registerLocaleData('es-419', localeData);
-
-      // And confirm that's what is returned
-      const newTitle = format.getFormatted(str_(UIStrings.title), 'es-419');
-      expect(newTitle).toEqual('new string for es-419 uses https!');
-
-      // Meanwhile another string that wasn't set in registerLocaleData just falls back to english
-      const newFailureTitle = format.getFormatted(str_(UIStrings.failureTitle), 'es-419');
-      expect(newFailureTitle).toEqual('Does not use HTTPS');
-
-      // Restore overwritten strings to avoid messing with other tests
-      locales['es-419'] = clonedLocales['es-419'];
-      const title = format.getFormatted(str_(UIStrings.title), 'es-419');
-      expect(title).toEqual('Usa HTTPS');
+    [
+      {label: 'relative path', filename: 'core/audits/is-on-https.js'},
+      {label: 'absolute path', filename: `${LH_ROOT}/core/audits/is-on-https.js`},
+    ].forEach(({label, filename}) => {
+      it(`overwrites existing locale strings: ${label}`, async () => {
+        const {UIStrings} = await import('../../../core/audits/is-on-https.js');
+        const str_ = i18n.createIcuMessageFn(filename, UIStrings);
+
+        // To start with, we get back the intended string..
+        const origTitle = format.getFormatted(str_(UIStrings.title), 'es-419');
+        expect(origTitle).toEqual('Usa HTTPS');
+        const origFailureTitle = format.getFormatted(str_(UIStrings.failureTitle), 'es-419');
+        expect(origFailureTitle).toEqual('No usa HTTPS');
+
+        // Now we declare and register the new string...
+        const localeData = {
+          'core/audits/is-on-https.js | title': {
+            'message': 'new string for es-419 uses https!',
+          },
+        };
+        format.registerLocaleData('es-419', localeData);
+
+        // And confirm that's what is returned
+        const newTitle = format.getFormatted(str_(UIStrings.title), 'es-419');
+        expect(newTitle).toEqual('new string for es-419 uses https!');
+
+        // Meanwhile another string that wasn't set in registerLocaleData just falls back to english
+        const newFailureTitle = format.getFormatted(str_(UIStrings.failureTitle), 'es-419');
+        expect(newFailureTitle).toEqual('Does not use HTTPS');
+
+        // Restore overwritten strings to avoid messing with other tests
+        locales['es-419'] = clonedLocales['es-419'];
+        const title = format.getFormatted(str_(UIStrings.title), 'es-419');
+        expect(title).toEqual('Usa HTTPS');
+      });
     });
   });
 
