pr fixes

connorjclark · connorjclark · commit 3b35d53608df · 2022-12-12T13:47:40.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,26 +194,11 @@ function createIcuMessageFn(filename, fileStrings) {
     const keyname = Object.keys(mergedStrings).find(key => mergedStrings[key] === message);
     if (!keyname) throw new Error(`Could not locate: ${message}`);
 
-    let filenameToLookup;
-    if (path.isAbsolute(filename)) {
-      // `message` can be a UIString defined within the provided `fileStrings`, or it could be
-      // one of the common strings found in `i18n.UIStrings`.
-      if (keyname in fileStrings) {
-        filenameToLookup = filename;
-      } else if (keyname in UIStrings) {
-        filenameToLookup = getModulePath(import.meta);
-      } else {
-        throw new Error(`Provided UIString is invalid: ${filename}, ${keyname}.`);
-      }
-
-      filenameToLookup = path.relative(LH_ROOT, filenameToLookup);
-    } else {
-      // `filename` might have been passed in as the exact i18n identifier
-      // already (see: stack-packs.js). Also the case for bundled lighthouse.
-      // Otherwise, the common case requires relativizing the absolute filename with LH_ROOT.
-      filenameToLookup = filename;
-    }
-
+    // `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}`;
 
diff --git a/shared/test/localization/format-test.js b/shared/test/localization/format-test.js
@@ -186,37 +186,41 @@ describe('format', () => {
       expect(formattedStr).toEqual('en-XZ cuerda!');
     });
 
-    it('overwrites existing locale strings', async () => {
-      const filename = `${LH_ROOT}/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');
+      });
     });
   });
 
