core(i18n): fix path bug resulting in invalid i18n id via npx #14314
Conversation
core/lib/i18n/i18n.js
Outdated
| @@ -189,8 +189,14 @@ 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, '/'); | |||
| let filenameToLookup = keyname in fileStrings ? filename : getModulePath(import.meta); | |||
There was a problem hiding this comment.
btw, any idea why this check/fallback to getModulePath (was __filename) is needed?
couldn't find any reasoning looking at the initial commit https://github.com/GoogleChrome/lighthouse/pull/5644/files#diff-1ca0a127c39d556edaeb5e53e6a12b74eaf76095850329302e48b24af41225d5R76
and i18n unit tests do not fail without this code.
There was a problem hiding this comment.
Ah, it allows usages of str_ to reference i18n strings not defined in the "local context".
There was a problem hiding this comment.
Attempted a more direct/readable way to do the same.
core/lib/i18n/i18n.js
Outdated
| const filenameToLookup = keyname in fileStrings ? filename : getModulePath(import.meta); | ||
| const unixStyleFilename = path.relative(LH_ROOT, filenameToLookup).replace(/\\/g, '/'); | ||
| let filenameToLookup; | ||
| if (path.isAbsolute(filename)) { |
There was a problem hiding this comment.
Good catch. Yikes, we messed this up :(
There was a problem hiding this comment.
Good catch. Yikes, we messed this up :(
Really this all only works because we manually set the stack-packs path to start with node_modules/ :/ I assume publisher-ads message IDs don't work, but it doesn't appear they have anything but english strings for now, so at least it's not observable.
Really we should have thought through the lighthouse dependency vs peer dependency case. I can't think of a universally workable system except if everyone (including us) switched to a package-name/path/to/file.js for message IDs, though.
Not something we need to fix today, but we'll need to if e.g. pubAds wants to translate their strings, plugins take off, etc
2402e02 to
3b35d53
Compare
| // one of the common strings found in `i18n.UIStrings`. | ||
| const filenameToLookup = keyname in fileStrings ? | ||
| filename : | ||
| path.relative(LH_ROOT, getModulePath(import.meta)); |
There was a problem hiding this comment.
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
Fixes #14248