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(tsc): fix audit details type hierarchy #7177

Merged
merged 5 commits into from
Feb 8, 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
17 changes: 14 additions & 3 deletions lighthouse-core/audits/accessibility/axe-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ class AxeAudit extends Audit {
const impact = rule && rule.impact;
const tags = rule && rule.tags;

/** @type {Array<{node: LH.Audit.DetailsRendererNodeDetailsJSON}>} */
/** @type {LH.Audit.Details.Table['items']}>} */
let items = [];
if (rule && rule.nodes) {
items = rule.nodes.map(node => ({
node: /** @type {LH.Audit.DetailsRendererNodeDetailsJSON} */ ({
node: /** @type {LH.Audit.Details.NodeValue} */ ({
type: 'node',
selector: Array.isArray(node.target) ? node.target.join(' ') : '',
path: node.path,
Expand All @@ -58,16 +58,27 @@ class AxeAudit extends Audit {
}));
}

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'node', itemType: 'node', text: str_(UIStrings.failingElementsHeader)},
];

/** @type {LH.Audit.Details.Diagnostic|undefined} */
let diagnostic;
if (impact || tags) {
Copy link
Member Author

Choose a reason for hiding this comment

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

left undefined if these weren't defined to match current LHR behavior

diagnostic = {
type: 'diagnostic',
impact,
tags,
};
}

return {
rawValue: typeof rule === 'undefined',
extendedInfo: {
value: rule,
},
details: {...Audit.makeTableDetails(headings, items), impact, tags},
details: {...Audit.makeTableDetails(headings, items), diagnostic},
};
}
}
Expand Down
12 changes: 6 additions & 6 deletions lighthouse-core/audits/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,10 @@ class Audit {
}

/**
* @param {Array<LH.Audit.Heading>} headings
* @param {Array<Object<string, LH.Audit.DetailsItem>>} results
* @param {LH.Audit.DetailsRendererDetailsSummary=} summary
* @return {LH.Audit.DetailsRendererDetailsJSON}
* @param {LH.Audit.Details.Table['headings']} headings
* @param {LH.Audit.Details.Table['items']} results
* @param {LH.Audit.Details.Table['summary']=} summary
* @return {LH.Audit.Details.Table}
*/
static makeTableDetails(headings, results, summary) {
if (results.length === 0) {
Expand All @@ -126,8 +126,8 @@ class Audit {
}

/**
* @param {Array<LH.Audit.Details.OpportunityColumnHeading>} headings
* @param {Array<LH.Audit.Details.OpportunityItem>} items
* @param {LH.Audit.Details.Opportunity['headings']} headings
* @param {LH.Audit.Details.Opportunity['items']} items
* @param {number} overallSavingsMs
* @param {number=} overallSavingsBytes
* @return {LH.Audit.Details.Opportunity}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/bootup-time.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ class BootupTime extends Audit {

const summary = {wastedMs: totalBootupTime};

/** @type {LH.Audit.Details.Table['headings']} */
Copy link
Member Author

@brendankenny brendankenny Feb 7, 2019

Choose a reason for hiding this comment

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

had to add a ton of these explicit headings type lines because the heading itemType now has a more stringent type (union of literals instead of just string) which won't work if it's been widened to string. There's an upcoming tsc 3.4 feature ("as const") that may make this a bit less of an issue all over the codebase, but this doesn't seem too bad for now

const headings = [
{key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},
{key: 'total', granularity: 1, itemType: 'ms', text: str_(UIStrings.columnTotal)},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ class TotalByteWeight extends ByteEfficiencyAudit {
context.options.scoreMedian
);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},
{key: 'totalBytes', itemType: 'bytes', text: str_(i18n.UIStrings.columnSize)},
Expand Down
16 changes: 12 additions & 4 deletions lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,12 +237,19 @@ class CacheHeaders extends Audit {
totalWastedBytes += wastedBytes;
if (url.includes('?')) queryStringCount++;

// Include cacheControl info (if it exists) per url as a diagnostic.
/** @type {LH.Audit.Details.Diagnostic|undefined} */
let diagnostic;
if (cacheControl) {
diagnostic = {
type: 'diagnostic',
...cacheControl,
};
}

results.push({
url,
// Include cacheControl in results, but cast as any so table types
// are happy. cacheControl is not shown in the table so this is OK.
// TODO(bckenny): fix DetailsItem
cacheControl: /** @type {any} */ (cacheControl),
diagnostic,
cacheLifetimeMs: cacheLifetimeInSeconds * 1000,
cacheHitProbability,
totalBytes,
Expand All @@ -260,6 +267,7 @@ class CacheHeaders extends Audit {
context.options.scoreMedian
);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},
// TODO(i18n): pre-compute localized duration
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/critical-request-chains.js
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ class CriticalRequestChains extends Audit {
},
},
details: {
type: 'criticalrequestchain',
type: /** @type {'criticalrequestchain'} */('criticalrequestchain'),
chains: flattenedChains,
longestChain,
},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/deprecations.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class Deprecations extends Audit {
};
});

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'value', itemType: 'code', text: 'Deprecation / Warning'},
{key: 'url', itemType: 'url', text: 'URL'},
Expand Down
6 changes: 5 additions & 1 deletion lighthouse-core/audits/diagnostics.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ class Diagnostics extends Audit {
return {
score: 1,
rawValue: 1,
details: {items: [diagnostics]},
details: {
type: 'diagnostic',
// TODO: Consider not nesting diagnostics under `items`.
items: [diagnostics],
Copy link
Collaborator

Choose a reason for hiding this comment

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

awww this is just to remain non-breaking? :)

the idea would be details: {type: 'diagnostic', ...diagnostics} if we were writing it today right?

Copy link
Member Author

Choose a reason for hiding this comment

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

awww this is just to remain non-breaking?

Ha, well, since we just added this and we consider it internal only, I'm happy to change, I just didn't want to assume and/or cause churn in people's lives, especially because i mostly don't use these :)

We could change these newer ones and leave the older ones like metrics until v5 since we know of at least a few people who rely on them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant "awwww" as in how thoughtful :D I'm probably the only person that relies on this and that would indeed be a pain, lol

I like leaving them until v5 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

haha, got it. SGTM :)

},
};
}
}
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/audits/dobetterweb/dom-size.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,14 @@ class DOMSize extends Audit {
context.options.scoreMedian
);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'statistic', itemType: 'text', text: str_(UIStrings.columnStatistic)},
{key: 'element', itemType: 'code', text: str_(UIStrings.columnElement)},
{key: 'value', itemType: 'numeric', text: str_(UIStrings.columnValue)},
];

/** @type {Array<Object<string, LH.Audit.DetailsItem>>} */
/** @type {LH.Audit.Details.Table['items']} */
const items = [
{
statistic: str_(UIStrings.statisticDOMNodes),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit {
};
});

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'href', itemType: 'url', text: 'URL'},
{key: 'target', itemType: 'text', text: 'Target'},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/dobetterweb/geolocation-on-start.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class GeolocationOnStart extends ViolationAudit {
// 'Only request geolocation information in response to a user gesture.'
const results = ViolationAudit.getViolationResults(artifacts, /geolocation/);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
{key: 'label', itemType: 'text', text: 'Location'},
Expand Down
5 changes: 3 additions & 2 deletions lighthouse-core/audits/dobetterweb/js-libraries.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,11 @@ class JsLibrariesAudit extends Audit {
static audit(artifacts) {
const libDetails = artifacts.JSLibraries.map(lib => ({
name: lib.name,
version: lib.version, // null if not detected
npm: lib.npmPkgName || null, // ~70% of libs come with this field
version: lib.version || undefined, // null if not detected
npm: lib.npmPkgName || undefined, // ~70% of libs come with this field
}));

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'name', itemType: 'text', text: 'Name'},
{key: 'version', itemType: 'text', text: 'Version'},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/dobetterweb/no-document-write.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class NoDocWriteAudit extends ViolationAudit {
static audit(artifacts) {
const results = ViolationAudit.getViolationResults(artifacts, /document\.write/);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
{key: 'label', itemType: 'text', text: 'Location'},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ class NoVulnerableLibrariesAudit extends Audit {
}

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

const libraryVulns = foundLibraries.map(lib => {
Expand Down Expand Up @@ -175,6 +175,7 @@ class NoVulnerableLibrariesAudit extends Audit {
displayValue = `${totalVulns} vulnerability detected`;
}

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'detectedLib', itemType: 'link', text: 'Library Version'},
{key: 'vulnCount', itemType: 'text', text: 'Vulnerability Count'},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class NotificationOnStart extends ViolationAudit {
static audit(artifacts) {
const results = ViolationAudit.getViolationResults(artifacts, /notification permission/);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
{key: 'label', itemType: 'text', text: 'Location'},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,15 @@ class PasswordInputsCanBePastedIntoAudit extends Audit {
static audit(artifacts) {
const passwordInputsWithPreventedPaste = artifacts.PasswordInputsWithPreventedPaste;

/** @type {Array<{node: LH.Audit.DetailsRendererNodeDetailsJSON}>} */
/** @type {LH.Audit.Details.Table['items']} */
const items = [];
passwordInputsWithPreventedPaste.forEach(input => {
items.push({
node: {type: 'node', snippet: input.snippet},
});
});

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'node', itemType: 'node', text: 'Failing Elements'},
];
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/dobetterweb/uses-http2.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ class UsesHTTP2Audit extends Audit {
displayValue = `${resources.length} request not served via HTTP/2`;
}

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
{key: 'protocol', itemType: 'text', text: 'Protocol'},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class PassiveEventsAudit extends ViolationAudit {
static audit(artifacts) {
const results = ViolationAudit.getViolationResults(artifacts, /passive event listener/);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
{key: 'label', itemType: 'text', text: 'Location'},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/errors-in-console.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class ErrorLogs extends Audit {

const tableRows = consoleRows.concat(runtimeExRows);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
{key: 'description', itemType: 'code', text: 'Description'},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/font-display.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ class FontDisplay extends Audit {
};
});

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},
{key: 'wastedMs', itemType: 'ms', text: str_(i18n.UIStrings.columnWastedMs)},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/image-aspect-ratio.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ class ImageAspectRatio extends Audit {
if (!processed.doRatiosMatch) results.push(processed);
});

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'thumbnail', text: ''},
{key: 'url', itemType: 'url', text: 'URL'},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/is-on-https.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class HTTPS extends Audit {

const items = Array.from(new Set(insecureURLs)).map(url => ({url}));

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: 'Insecure URL'},
];
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/main-thread-tasks.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class MainThreadTasks extends Audit {
};
});

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'type', itemType: 'text', text: 'Task Type'},
{key: 'startTime', itemType: 'ms', granularity: 1, text: 'Start Time'},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/mainthread-work-breakdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ class MainThreadWorkBreakdown extends Audit {
};
});

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'groupLabel', itemType: 'text', text: str_(UIStrings.columnCategory)},
{key: 'duration', itemType: 'ms', granularity: 1, text: str_(i18n.UIStrings.columnTimeSpent)},
Expand Down
10 changes: 6 additions & 4 deletions lighthouse-core/audits/metrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,12 @@ class Metrics extends Audit {
}
}

/** @type {MetricsDetails} */
const details = {items: [metrics]};
/** @type {LH.Audit.Details.Diagnostic} */
const details = {
type: 'diagnostic',
// TODO: Consider not nesting metrics under `items`.
items: [metrics],
};

return {
score: 1,
Expand Down Expand Up @@ -155,6 +159,4 @@ class Metrics extends Audit {
* @property {number} observedSpeedIndexTs
*/

/** @typedef {{items: [UberMetricsItem]}} MetricsDetails */

module.exports = Metrics;
1 change: 1 addition & 0 deletions lighthouse-core/audits/mixed-content.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ class MixedContent extends Audit {
const displayValue = `${Util.formatNumber(upgradeableResources.length)}
${upgradeableResources.length === 1 ? 'request' : 'requests'}`;

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'fullUrl', itemType: 'url', text: 'URL'},
];
Expand Down
8 changes: 7 additions & 1 deletion lighthouse-core/audits/multi-check-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,13 @@ class MultiCheckAudit extends Audit {
});
}

const details = {items: [detailsItem]};
// Include the detailed pass/fail checklist as a diagnostic.
/** @type {LH.Audit.Details.Diagnostic} */
const details = {
type: 'diagnostic',
// TODO: Consider not nesting detailsItem under `items`.
items: [detailsItem],
};

// If we fail, share the failures
if (result.failures.length > 0) {
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/network-requests.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ class NetworkRequests extends Audit {
};
});

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
{key: 'startTime', itemType: 'ms', granularity: 1, text: 'Start Time'},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/network-rtt.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class NetworkRTT extends Audit {

results.sort((a, b) => b.rtt - a.rtt);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'origin', itemType: 'text', text: str_(i18n.UIStrings.columnURL)},
{key: 'rtt', itemType: 'ms', granularity: 1, text: str_(i18n.UIStrings.columnTimeSpent)},
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/network-server-latency.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class NetworkServerLatency extends Audit {

results.sort((a, b) => b.serverReponseTime - a.serverReponseTime);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'origin', itemType: 'text', text: str_(i18n.UIStrings.columnURL)},
{key: 'serverReponseTime', itemType: 'ms', granularity: 1,
Expand Down
6 changes: 5 additions & 1 deletion lighthouse-core/audits/predictive-perf.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,11 @@ class PredictivePerf extends Audit {
score,
rawValue: values.roughEstimateOfTTI,
displayValue: Util.formatMilliseconds(values.roughEstimateOfTTI),
details: {items: [values]},
details: {
type: 'diagnostic',
// TODO: Consider not nesting values under `items`.
items: [values],
},
};
}
}
Expand Down
Loading