Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Dec 12, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
18e05da
core(i18n): fix path bug resulting in invalid i18n id
connorjclark Aug 23, 2022
909fe52
explicit
connorjclark Aug 23, 2022
1328e2e
update
connorjclark Aug 23, 2022
ee75f1b
update
connorjclark Aug 23, 2022
d073fc6
update
connorjclark Aug 23, 2022
5bc10d0
fix test
connorjclark Aug 23, 2022
cefe2f2
fine
connorjclark Aug 23, 2022
3b35d53
pr fixes
connorjclark Dec 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions core/lib/i18n/i18n.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>}
Expand All @@ -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));
Copy link
Collaborator Author

@connorjclark connorjclark Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no real change - just duping some work because code is clearer. It's obvious that getModulePath always returns an absolute path; but that the filename passed in could be absolute or relative is not so clear, and better to normalize parameters at start of functions. so...relativize in two places

const unixStyleFilename = filenameToLookup.replace(/\\/g, '/');
const i18nId = `${unixStyleFilename} | ${keyname}`;

return {
Expand Down
67 changes: 36 additions & 31 deletions shared/test/localization/format-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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');
});
});
});

Expand Down