core(tsc): fix audit details type hierarchy #7177

brendankenny · 2019-02-07T06:07:37Z

Finally maps out types for the details objects!

This PR was already getting long enough, so this (mostly) only touches the production of details object in audits. I'll follow up with a change to make details-renderer.js and etc use these types. I already have that working in a branch, so I promise it will work on that side as well :)

For the major thrust of things see audit-details.d.ts (and what was deleted from audit.d.ts). Everything else is mostly just switching over to use the types, not changes to the audits themselves.

The new details structure just describes what we currently do: there are a few details object shapes that we use directly (criticalrequestchain, filmstrip, opportunity, screenshot, table, and now diagnostic), then a bunch of things that are only ever used inside those details (text, url, 'code', etc). If and when we need some of those code or whatever as top-level details, it's easy to add them to the union of details output by audits, just like it's easy to add additional properties.

There are two relatively small changes to audit output:

no more null in details. This was a minor thing, but we only used it in two non-visible audits,
as discussed in chat, a new 'diagnostic' type. There are plenty of places you can still stick values that aren't visible in the final report in details (like unreferenced properties on table items), but this is the place to stick full-on objects or entire details objects like metrics.js or diagnostics.js. It's an explicit signal that a property won't be in the html report, and it's a way of keeping the details types relatively narrow so that type checking is useful (you can still put anything in them, but they just have to be labeled with type: 'diagnostic').

In practice the effect of both are relatively minor (at most you get some diagnostic data one level deeper in the a11y audits). Check out the changes to sample_v2.json for all the LHR changes that occur.

brendankenny · 2019-02-07T06:10:59Z

types/audit-details.d.ts

+          wastedMs?: number;
+          wastedBytes?: number;
+        };
+        diagnostic?: Diagnostic;


turns out I couldn't do [p: string]: Diagnostic here because that means any string accessor of Table should return a value assignable to Diagnostic, which we don't want. So this is a middle ground, and seems flexible enough (@exterkamp rightly pointed out that if a Diagnostic object can contain anything, why would you ever need two of them as properties on the same object?)

being able to define a set of properties with certain types and a different type for every other property not explicitly defined will be possible if/when negated types land in tsc (microsoft/TypeScript#29317)

brendankenny · 2019-02-07T06:14:52Z

types/audit-details.d.ts

+        totalBytes?: number;
+        wastedMs?: number;
+        diagnostic?: Diagnostic;
+        [p: string]: number | boolean | string | undefined | Diagnostic;


otoh, any TableItem or OpportunityItem property can be a Diagnostic because of how loose we allow the default type to be (almost but not quite any)

brendankenny · 2019-02-07T06:15:57Z

types/audit-details.d.ts


-    // Contents of details below here
+      // TODO(bckenny): unify Table/Opportunity headings and items on next breaking change.


TableColumnHeading and OpportunityColumnHeading are very close but not close enough (e.g. label vs text). I think we just didn't get around to making them the same :S

Something for v5 :)

brendankenny · 2019-02-07T06:18:25Z

lighthouse-core/audits/bootup-time.js

@@ -159,6 +159,7 @@ class BootupTime extends Audit {

    const summary = {wastedMs: totalBootupTime};

+    /** @type {LH.Audit.Details.Table['headings']} */


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

brendankenny · 2019-02-07T06:23:40Z

lighthouse-core/audits/accessibility/axe-audit.js

    const headings = [
      {key: 'node', itemType: 'node', text: str_(UIStrings.failingElementsHeader)},
    ];

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


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

patrickhulce · 2019-02-07T15:38:27Z

lighthouse-core/audits/diagnostics.js

+      details: {
+        type: 'diagnostic',
+        // TODO: Consider not nesting diagnostics under `items`.
+        items: [diagnostics],


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

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

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.

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 :)

haha, got it. SGTM :)

patrickhulce

LGTM! I can actually understand the details type organization now! :D

patrickhulce · 2019-02-08T00:59:05Z

lighthouse-core/audits/multi-check-audit.js

@@ -11,6 +11,15 @@

 const Audit = require('./audit');

+/** @typedef {
+  Partial<Record<LH.Artifacts.ManifestValueCheckID, boolean>> &


Is there a long-term plan here with the multi-check audits?

I'm not sure the js typedefs are a readability win in this step.

Is there a long-term plan here with the multi-check audits?
I'm not sure the js typedefs are a readability win in this step.

ah, now that you say that, there's no reason it can't keep living in audit.d.ts. I was deleting all the details types from there, but there's no reason to consider it a details type now, it's just a handy typedef. I'll put it back.

(personally I hate multi-check-audit and hope it dies as a base class someday, but that's more the long-long-term plan :)

patrickhulce · 2019-02-08T01:01:40Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

@@ -160,8 +160,9 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
    const thumbnailResult = thumbnailAudit && thumbnailAudit.result;
    if (thumbnailResult && thumbnailResult.details) {
      timelineEl.id = thumbnailResult.id;
+      // @ts-ignore TODO(bckenny): fix detailsRenderer.render input type


input type? output type looks busted too based on the next line change 😆

input type? output type looks busted too based on the next line change

haha, maybe both then

paulirish

i think this is great.

paulirish · 2019-02-07T20:29:09Z

types/audit.d.ts

@@ -155,7 +101,7 @@ declare global {
      /** A more detailed description that describes why the audit is important and links to Lighthouse documentation on the audit; markdown links supported. */
      description: string;
      // TODO(bckenny): define details
-      details?: any;


exterkamp

LGTM ❤️ making details better!

brendankenny added 3 commits February 6, 2019 18:24

core(tsc): canonicalize audit details types

3775cb3

ts docs

896aafc

sample json

c29a294

brendankenny requested review from exterkamp, patrickhulce and paulirish as code owners February 7, 2019 06:07

brendankenny commented Feb 7, 2019

View reviewed changes

revert annotation and ts-ignore comments

f82f331

patrickhulce reviewed Feb 7, 2019

View reviewed changes

patrickhulce approved these changes Feb 8, 2019

View reviewed changes

paulirish approved these changes Feb 8, 2019

View reviewed changes

multipass

7f38828

exterkamp approved these changes Feb 8, 2019

View reviewed changes

brendankenny merged commit c79e357 into master Feb 8, 2019

brendankenny deleted the details branch February 8, 2019 20:01

brendankenny mentioned this pull request Feb 9, 2019

report(details-renderer): use new details types #7192

Merged

This was referenced Feb 22, 2019

reportCategories.audits.result.details.header does not have a consistent type #4140

Closed

Duplicated DetailsRenderer.NodeDetailsJSON type definition #3360

Closed

brendankenny mentioned this pull request Apr 17, 2020

misc: minor cleanup of audit-details type names #10603

Merged

connorjclark mentioned this pull request Sep 19, 2022

report: consolidate table headers #14315

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(tsc): fix audit details type hierarchy #7177

core(tsc): fix audit details type hierarchy #7177

brendankenny commented Feb 7, 2019

brendankenny Feb 7, 2019

brendankenny Feb 7, 2019

brendankenny Feb 7, 2019

brendankenny Feb 7, 2019 •

edited

Loading

brendankenny Feb 7, 2019 •

edited

Loading

brendankenny Feb 7, 2019

patrickhulce Feb 7, 2019

brendankenny Feb 7, 2019

patrickhulce Feb 7, 2019

brendankenny Feb 7, 2019

patrickhulce left a comment

patrickhulce Feb 8, 2019

brendankenny Feb 8, 2019

patrickhulce Feb 8, 2019

brendankenny Feb 8, 2019

paulirish left a comment

paulirish Feb 7, 2019

exterkamp left a comment


		// Contents of details below here
		// TODO(bckenny): unify Table/Opportunity headings and items on next breaking change.

		@@ -159,6 +159,7 @@ class BootupTime extends Audit {

		const summary = {wastedMs: totalBootupTime};

		/** @type {LH.Audit.Details.Table['headings']} */

core(tsc): fix audit details type hierarchy #7177

core(tsc): fix audit details type hierarchy #7177

Conversation

brendankenny commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

brendankenny Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterkamp left a comment

Choose a reason for hiding this comment

brendankenny Feb 7, 2019 •

edited

Loading

brendankenny Feb 7, 2019 •

edited

Loading