-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Yikes, we messed this up :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
Fixes #14248