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

i18n: use IcuMessage objects instead of string IDs #10630

Merged
merged 8 commits into from
Sep 17, 2020
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
10 changes: 7 additions & 3 deletions clients/test/extension/settings-controller-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,18 @@
/* eslint-env jest */

const SettingsController = require('../../extension/scripts/settings-controller.js');
const Config = require('../../../lighthouse-core/config/config.js');
const defaultConfig = require('../../../lighthouse-core/config/default-config.js');
const i18n = require('../../../lighthouse-core/lib/i18n/i18n.js');

describe('Lighthouse chrome extension SettingsController', () => {
it('default categories should be correct', () => {
const categories = Config.getCategories(defaultConfig);
categories.forEach(cat => cat.title = i18n.getFormatted(cat.title, 'en-US'));
const categories = Object.entries(defaultConfig.categories)
.map(([id, category]) => {
return {
id,
title: i18n.getFormatted(category.title, 'en-US'),
};
});
expect(SettingsController.DEFAULT_CATEGORIES).toEqual(categories);
});
});
7 changes: 3 additions & 4 deletions lighthouse-cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const assetSaver = require('../lighthouse-core/lib/asset-saver.js');

const open = require('open');

/** @typedef {import('../lighthouse-core/lib/lh-error.js')} LighthouseError */
/** @typedef {Error & {code: string, friendlyMessage?: string}} ExitError */
Copy link
Member Author

Choose a reason for hiding this comment

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

wasn't worth pretending this was a LighthouseError since either it's a regular Error or we construct it as just an object down on line 224 :)


const _RUNTIME_ERROR_CODE = 1;
const _PROTOCOL_TIMEOUT_EXIT_CODE = 67;
Expand Down Expand Up @@ -92,7 +92,7 @@ function printProtocolTimeoutErrorAndExit() {
}

/**
* @param {LighthouseError} err
* @param {ExitError} err
* @return {never}
*/
function printRuntimeErrorAndExit(err) {
Expand All @@ -104,7 +104,7 @@ function printRuntimeErrorAndExit(err) {
}

/**
* @param {LighthouseError} err
* @param {ExitError} err
* @return {never}
*/
function printErrorAndExit(err) {
Expand Down Expand Up @@ -239,7 +239,6 @@ async function runLighthouse(url, flags, config) {
return printErrorAndExit({
name: 'LHError',
friendlyMessage: runtimeError.message,
lhrRuntimeError: true,
code: runtimeError.code,
message: runtimeError.message,
});
Expand Down
6 changes: 3 additions & 3 deletions lighthouse-core/audits/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,8 @@ class Audit {

/**
* @param {typeof Audit} audit
* @param {string} errorMessage
* @return {LH.Audit.Result}
* @param {string | LH.IcuMessage} errorMessage
* @return {LH.RawIcu<LH.Audit.Result>}
*/
static generateErrorAuditResult(audit, errorMessage) {
return Audit.generateAuditResult(audit, {
Expand All @@ -255,7 +255,7 @@ class Audit {
/**
* @param {typeof Audit} audit
* @param {LH.Audit.Product} product
* @return {LH.Audit.Result}
* @return {LH.RawIcu<LH.Audit.Result>}
*/
static generateAuditResult(audit, product) {
if (product.score === undefined) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ const WASTED_MS_FOR_SCORE_OF_ZERO = 5000;
* @property {Array<LH.Audit.ByteEfficiencyItem>} items
* @property {Map<string, number>=} wastedBytesByUrl
* @property {LH.Audit.Details.Opportunity['headings']} headings
* @property {string} [displayValue]
* @property {string} [explanation]
* @property {Array<string>} [warnings]
* @property {LH.IcuMessage} [displayValue]
* @property {LH.IcuMessage} [explanation]
* @property {Array<string | LH.IcuMessage>} [warnings]
*/

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ class RenderBlockingResources extends Audit {
static async audit(artifacts, context) {
const {results, wastedMs} = await RenderBlockingResources.computeResults(artifacts, context);

let displayValue = '';
let displayValue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are inferred correctly, right? in tsc playground it seems to work out out OK but some of the assertions we're removing later on in jsdoc makes me think some of them were necessary at one point in time for our checkJs setup...

https://www.typescriptlang.org/play/#code/DYUwLgBAZg9jDcAoRBLKEAUBZAhmAFgHQBOOAdgCYwC2GAlBAHwQAMhArAwN6IR-RwIAXghcAHgC4IARgA0EAJ5SATAF9E65AGMYZAM4xQhYDADmGWDEIAvOkA

Copy link
Member Author

Choose a reason for hiding this comment

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

these are inferred correctly, right?

yes, they haven't been necessary for a while. I don't remember when conditional initialization like this was added, but basically the CFG typing keeps getting better.

if (results.length > 0) {
displayValue = str_(i18n.UIStrings.displayValueMsSavings, {wastedMs});
}
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/content-width.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class ContentWidth extends Audit {
};
}

let explanation = '';
let explanation;
if (!widthsMatch) {
explanation = str_(UIStrings.explanation,
{innerWidth: artifacts.ViewportDimensions.innerWidth,
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/deprecations.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class Deprecations extends Audit {
];
const details = Audit.makeTableDetails(headings, deprecations);

let displayValue = '';
let displayValue;
if (deprecations.length > 0) {
displayValue = str_(UIStrings.displayValue, {itemCount: deprecations.length});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit {
* @return {LH.Audit.Product}
*/
static audit(artifacts) {
/** @type {string[]} */
/** @type {LH.IcuMessage[]} */
const warnings = [];
const pageHost = new URL(artifacts.URL.finalUrl).host;
const failingAnchors = artifacts.AnchorElements
Expand Down
10 changes: 5 additions & 5 deletions lighthouse-core/audits/dobetterweb/no-vulnerable-libraries.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,15 @@ const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

const SEMVER_REGEX = /^(\d+\.\d+\.\d+)[^-0-9]+/;

/** @type {Object<string, string>} */
/** @type {Record<string, LH.IcuMessage>} */
const rowMap = {
'low': str_(UIStrings.rowSeverityLow),
'medium': str_(UIStrings.rowSeverityMedium),
'high': str_(UIStrings.rowSeverityHigh),
};

/** @typedef {{npm: Object<string, Array<{id: string, severity: string, semver: {vulnerable: Array<string>}}>>}} SnykDB */
/** @typedef {{severity: string, numericSeverity: number, library: string, url: string}} Vulnerability */
/** @typedef {{severity: LH.IcuMessage, numericSeverity: number, library: string, url: string}} Vulnerability */

class NoVulnerableLibrariesAudit extends Audit {
/**
Expand Down Expand Up @@ -158,7 +158,7 @@ class NoVulnerableLibrariesAudit extends Audit {

/**
* @param {Array<Vulnerability>} vulnerabilities
* @return {string}
* @return {LH.IcuMessage}
*/
static highestSeverity(vulnerabilities) {
const sortedVulns = vulnerabilities
Expand All @@ -181,7 +181,7 @@ class NoVulnerableLibrariesAudit extends Audit {
}

let totalVulns = 0;
/** @type {Array<{highestSeverity: string, vulnCount: number, detectedLib: LH.Audit.Details.LinkValue}>} */
/** @type {Array<{highestSeverity: LH.IcuMessage, vulnCount: number, detectedLib: LH.Audit.Details.LinkValue}>} */
const vulnerabilityResults = [];

for (const lib of foundLibraries) {
Expand All @@ -206,7 +206,7 @@ class NoVulnerableLibrariesAudit extends Audit {
}
}

let displayValue = '';
let displayValue;
if (totalVulns > 0) {
displayValue = str_(UIStrings.displayValue, {itemCount: totalVulns});
}
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/dobetterweb/uses-http2.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ class UsesHTTP2Audit extends Audit {
const resources = UsesHTTP2Audit.determineNonHttp2Resources(networkRecords);
const wastedMs = UsesHTTP2Audit.computeWasteWithTTIGraph(resources, graph, simulator);

let displayValue = '';
let displayValue;
if (resources.length > 0) {
displayValue = str_(UIStrings.displayValue, {itemCount: resources.length});
}
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/font-display.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class FontDisplay extends Audit {
/**
* Some pages load many fonts we can't check, so dedupe on origin.
* @param {Array<string>} warningUrls
* @return {Array<string>}
* @return {Array<LH.IcuMessage>}
*/
static getWarningsForFontUrls(warningUrls) {
/** @type {Map<string, number>} */
Expand Down
10 changes: 5 additions & 5 deletions lighthouse-core/audits/image-aspect-ratio.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class ImageAspectRatio extends Audit {

/**
* @param {WellDefinedImage} image
* @return {Error|{url: string, displayedAspectRatio: string, actualAspectRatio: string, doRatiosMatch: boolean}}
* @return {LH.IcuMessage|{url: string, displayedAspectRatio: string, actualAspectRatio: string, doRatiosMatch: boolean}}
*/
static computeAspectRatios(image) {
const url = URL.elideDataURI(image.src);
Expand All @@ -68,7 +68,7 @@ class ImageAspectRatio extends Audit {

if (!Number.isFinite(actualAspectRatio) ||
!Number.isFinite(displayedAspectRatio)) {
return new Error(str_(UIStrings.warningCompute, {url}));
return str_(UIStrings.warningCompute, {url});
Copy link
Member Author

Choose a reason for hiding this comment

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

we never use this as an Error, just a string container (the Error is returned and then if it passes instanceof Error, its message is pushed into a warnings array), so if replacing that string with an IcuMessage, might as well just use that as the whole return value in the error case :)

}

return {
Expand All @@ -88,7 +88,7 @@ class ImageAspectRatio extends Audit {
static audit(artifacts) {
const images = artifacts.ImageElements;

/** @type {string[]} */
/** @type {LH.IcuMessage[]} */
const warnings = [];
/** @type {Array<{url: string, displayedAspectRatio: string, actualAspectRatio: string, doRatiosMatch: boolean}>} */
const results = [];
Expand All @@ -109,8 +109,8 @@ class ImageAspectRatio extends Audit {
}).forEach(image => {
const wellDefinedImage = /** @type {WellDefinedImage} */ (image);
const processed = ImageAspectRatio.computeAspectRatios(wellDefinedImage);
if (processed instanceof Error) {
warnings.push(processed.message);
if (i18n.isIcuMessage(processed)) {
warnings.push(processed);
return;
}

Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/audits/is-on-https.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class HTTPS extends Audit {
.filter(record => !HTTPS.isSecureRecord(record))
.map(record => URL.elideDataURI(record.url));

/** @type {Array<{url: string, resolution?: string}>} */
/** @type {Array<{url: string, resolution?: LH.IcuMessage|string}>} */
const items = Array.from(new Set(insecureURLs)).map(url => ({url, resolution: undefined}));

/** @type {LH.Audit.Details.Table['headings']} */
Expand Down Expand Up @@ -119,7 +119,7 @@ class HTTPS extends Audit {
if (!item.resolution) item.resolution = str_(UIStrings.allowed);
}

let displayValue = '';
let displayValue;
if (items.length > 0) {
displayValue = str_(UIStrings.displayValue, {itemCount: items.length});
}
Expand Down
2 changes: 0 additions & 2 deletions lighthouse-core/audits/load-fast-enough-for-pwa.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,7 @@ class LoadFastEnough4Pwa extends Audit {

const score = Number(tti.timing < MAXIMUM_TTI);

/** @type {string|undefined} */
let displayValue;
/** @type {string|undefined} */
let explanation;
if (!score) {
displayValue = str_(displayValueTemplate, {timeInMs: tti.timing});
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/non-composited-animations.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ const ACTIONABLE_FAILURE_REASONS = [
* We can check if a specific bit is true in the failure coding using bitwise and '&' with the flag.
* @param {number} failureCode
* @param {string[]} unsupportedProperties
* @return {string[]}
* @return {LH.IcuMessage[]}
*/
function getActionableFailureReasons(failureCode, unsupportedProperties) {
return ACTIONABLE_FAILURE_REASONS
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/offline-start-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class OfflineStartUrl extends Audit {
// The StartUrl artifact provides no explanation if a response was received from a service
// worker with a non-200 status code. In that case, this audit provides its own explanation.
// In all other cases it defers to the artifact explanation.
/** @type {string|LH.IcuMessage|undefined} */
let explanation = artifacts.StartUrl.explanation;
if (!explanation && artifacts.StartUrl.statusCode !== -1 && !hasOfflineStartUrl) {
explanation = str_(UIStrings.errorLoading, {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/performance-budget.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const UIStrings = {
const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

/** @typedef {import('../computed/resource-summary.js').ResourceEntry} ResourceEntry */
/** @typedef {{resourceType: LH.Budget.ResourceType, label: string, requestCount: number, transferSize: number, sizeOverBudget: number | undefined, countOverBudget: string | undefined}} BudgetItem */
/** @typedef {{resourceType: LH.Budget.ResourceType, label: LH.IcuMessage, requestCount: number, transferSize: number, sizeOverBudget: number | undefined, countOverBudget: LH.IcuMessage | undefined}} BudgetItem */

class ResourceBudget extends Audit {
/**
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/resource-summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class ResourceSummary extends Audit {
];


/** @type {Record<LH.Budget.ResourceType,string>} */
/** @type {Record<LH.Budget.ResourceType, LH.IcuMessage>} */
const strMappings = {
'total': str_(i18n.UIStrings.totalResourceType),
'document': str_(i18n.UIStrings.documentResourceType),
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/seo/hreflang.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

/** @typedef {string|LH.Audit.Details.NodeValue|undefined} Source */
/** @typedef {{source: Source, subItems: {type: 'subitems', items: SubItem[]}}} InvalidHreflang */
/** @typedef {{reason: string}} SubItem */
/** @typedef {{reason: LH.IcuMessage}} SubItem */

const Audit = require('../audit.js');
const VALID_LANGS = importValidLangs();
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/service-worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class ServiceWorker extends Audit {
* contolled by the scopeUrl.
* @param {LH.Artifacts['WebAppManifest']} WebAppManifest
* @param {string} scopeUrl
* @return {string|undefined}
* @return {LH.IcuMessage|undefined}
*/
static checkStartUrl(WebAppManifest, scopeUrl) {
if (!WebAppManifest) {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/third-party-summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const PASS_THRESHOLD_IN_MS = 250;
* @typedef {{
* transferSize: number,
* blockingTime: number,
* url: string,
* url: string | LH.IcuMessage,
* }} URLSummary
*/

Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/audits/timing-budget.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const UIStrings = {

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

/** @typedef {{metric: LH.Budget.TimingMetric, label: string, measurement?: LH.Audit.Details.NumericValue|number, overBudget?: LH.Audit.Details.NumericValue|number}} BudgetItem */
/** @typedef {{metric: LH.Budget.TimingMetric, label: LH.IcuMessage, measurement?: LH.Audit.Details.NumericValue|number, overBudget?: LH.Audit.Details.NumericValue|number}} BudgetItem */

class TimingBudget extends Audit {
/**
Expand All @@ -42,7 +42,7 @@ class TimingBudget extends Audit {

/**
* @param {LH.Budget.TimingMetric} timingMetric
* @return {string}
* @return {LH.IcuMessage}
*/
static getRowLabel(timingMetric) {
/** @type {Record<LH.Budget.TimingMetric, string>} */
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/audits/user-timings.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ class UserTimings extends Audit {

const details = Audit.makeTableDetails(headings, tableRows);

/** @type {string|undefined} */
let displayValue;
if (userTimings.length) {
displayValue = str_(UIStrings.displayValue, {itemCount: userTimings.length});
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/uses-rel-preconnect.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class UsesRelPreconnectAudit extends Audit {
const settings = context.settings;

let maxWasted = 0;
/** @type {string[]} */
/** @type {Array<LH.IcuMessage>} */
const warnings = [];

const [networkRecords, mainResource, loadSimulator, traceOfTab, pageGraph] = await Promise.all([
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/uses-rel-preload.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ class UsesRelPreloadAudit extends Audit {
// sort results by wastedTime DESC
results.sort((a, b) => b.wastedMs - a.wastedMs);

/** @type {Array<string>|undefined} */
/** @type {Array<LH.IcuMessage>|undefined} */
let warnings;
const failedURLs = UsesRelPreloadAudit.getURLsFailedToPreload(graph);
if (failedURLs.size) {
Expand Down
7 changes: 4 additions & 3 deletions lighthouse-core/config/config-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
const path = require('path');
const Audit = require('../audits/audit.js');
const Runner = require('../runner.js');
const i18n = require('../lib/i18n/i18n.js');

/**
* If any items with identical `path` properties are found in the input array,
Expand Down Expand Up @@ -53,19 +54,19 @@ function assertValidAudit(auditDefinition) {
throw new Error(`${auditName} has no meta.id property, or the property is not a string.`);
}

if (typeof implementation.meta.title !== 'string') {
if (!i18n.isStringOrIcuMessage(implementation.meta.title)) {
throw new Error(`${auditName} has no meta.title property, or the property is not a string.`);
}

// If it'll have a ✔ or ✖ displayed alongside the result, it should have failureTitle
if (
typeof implementation.meta.failureTitle !== 'string' &&
!i18n.isStringOrIcuMessage(implementation.meta.failureTitle) &&
implementation.meta.scoreDisplayMode === Audit.SCORING_MODES.BINARY
) {
throw new Error(`${auditName} has no failureTitle and should.`);
}

if (typeof implementation.meta.description !== 'string') {
if (!i18n.isStringOrIcuMessage(implementation.meta.description)) {
throw new Error(
`${auditName} has no meta.description property, or the property is not a string.`
);
Expand Down
Loading