Skip to content
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

core(i18n): throw on excess placeholder replacement values #9580

Merged
merged 1 commit into from
Aug 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion lighthouse-core/audits/third-party-summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,6 @@ class ThirdPartySummary extends Audit {
return {
score: Number(summary.wastedMs <= PASS_THRESHOLD_IN_MS),
displayValue: str_(UIStrings.displayValue, {
itemCount: results.length,
timeInMs: summary.wastedMs,
}),
details: Audit.makeTableDetails(headings, results, summary),
Expand Down
11 changes: 11 additions & 0 deletions lighthouse-core/lib/i18n/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,17 @@ function _processParsedElements(icuMessage, argumentElements, values = {}) {
}
}

// Throw an error if a value is provided but has no placeholder in the message.
for (const valueId of Object.keys(values)) {
// errorCode is a special case always allowed to help ease of LHError use.
if (valueId === 'errorCode') continue;

if (!argumentElements.find(el => el.id === valueId)) {
throw new Error(`Provided value "${valueId}" does not match any placeholder in ` +
`ICU message "${icuMessage}"`);
}
}

return values;
}

Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ class LighthouseError extends Error {
super(errorDefinition.code);
this.name = 'LHError';
this.code = errorDefinition.code;
// Insert the i18n reference with errorCode and all additional ICU replacement properties.
// Add additional properties to be ICU replacements in the error string.
// `code` is always added as `errorCode` so callers don't need to specify the code multiple times.
this.friendlyMessage = str_(errorDefinition.message, {errorCode: this.code, ...properties});
this.lhrRuntimeError = !!errorDefinition.lhrRuntimeError;
if (properties) Object.assign(this, properties);
Expand Down
25 changes: 20 additions & 5 deletions lighthouse-core/test/lib/i18n/i18n-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,19 @@ describe('i18n', () => {

describe('#replaceIcuMessageInstanceIds', () => {
it('replaces the references in the LHR', () => {
const templateID = 'lighthouse-core/test/lib/i18n/fake-file.js | daString';
const reference = templateID + ' # 0';
const lhr = {audits: {'fake-audit': {title: reference}}};
const fakeFile = path.join(__dirname, 'fake-file-number-2.js');
const UIStrings = {aString: 'different {x}!'};
const formatter = i18n.createMessageInstanceIdFn(fakeFile, UIStrings);

const title = formatter(UIStrings.aString, {x: 1});
const lhr = {audits: {'fake-audit': {title}}};

const icuMessagePaths = i18n.replaceIcuMessageInstanceIds(lhr, 'en-US');
expect(lhr.audits['fake-audit'].title).toBe('use me!');
expect(lhr.audits['fake-audit'].title).toBe('different 1!');

const expectedPathId = 'lighthouse-core/test/lib/i18n/fake-file-number-2.js | aString';
expect(icuMessagePaths).toEqual({
[templateID]: [{path: 'audits[fake-audit].title', values: {x: 1}}]});
[expectedPathId]: [{path: 'audits[fake-audit].title', values: {x: 1}}]});
Copy link
Member Author

Choose a reason for hiding this comment

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

dangers of singletons :) this test was implicitly relying on the test above having called createMessageInstanceIdFn

});
});

Expand Down Expand Up @@ -191,5 +196,15 @@ describe('i18n', () => {
// eslint-disable-next-line max-len
.toThrow(`ICU Message "Hello {timeInMs, number, seconds} World" contains a numeric reference ("timeInMs") but provided value was not a number`);
});

it('throws an error if a value is provided that has no placeholder in the message', () => {
const helloStr = str_(UIStrings.helloTimeInMsWorld, {
timeInMs: 55,
sirNotAppearingInThisString: 66,
});
expect(_ => i18n.getFormatted(helloStr, 'en-US'))
// eslint-disable-next-line max-len
Copy link
Collaborator

Choose a reason for hiding this comment

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

apparently we've decided there's disagreement on this point so we should never use disable-next-line max-len again :P

@exterkamp I'm gonna love this convo next week where I can learn how splitting is possibly better 😆

.toThrow(`Provided value "sirNotAppearingInThisString" does not match any placeholder in ICU message "Hello {timeInMs, number, seconds} World"`);
});
});
});
1 change: 0 additions & 1 deletion lighthouse-core/test/results/sample_v2.json
Original file line number Diff line number Diff line change
Expand Up @@ -5605,7 +5605,6 @@
"lighthouse-core/audits/third-party-summary.js | displayValue": [
{
"values": {
"itemCount": 1,
"timeInMs": 22.918000000000006
},
"path": "audits[third-party-summary].displayValue"
Expand Down