core(lhr): lhr-lite type declaration #4983

brendankenny · 2018-04-17T21:06:39Z

ready for feedback on design/implementation of the design :)

brendankenny · 2018-04-17T21:10:50Z

typings/lhr-lite.d.ts

+         * 'notApplicable': the audit turned out to not apply to the page. Ignore the score.
+         * 'manual': The audit exists only to tell you to review something yourself. Ignore the score.
+         */
+        scoreDisplayMode: 'binary' | 'numeric' | 'informative' | 'notApplicable' | 'manual';


should we have an 'error' scoreDisplayMode? That brings us (basically) back to a boolean and a string, which isn't ideal, but it also makes sense as it affects interpretation of the score in the same way that 'informative' 'notApplicable', and 'manual' do (you should ignore whatever is in score) and you won't have to look in multiple places to see how you should interpret score (like you do currently)

I like it 👍

paulirish · 2018-04-17T21:23:14Z

typings/lhr-lite.d.ts

+        wastedMs: number
+        wastedBytes?: number
+        headings: AuditColumnHeading[];
+        rows: AuditTableRow[];


renaming items to rows, eh? :)

I prefer items personally because it doesn't imply these table semantics. Table-ness is a visual/report thing, which i don't think is relevant to most consumers of the LHR

renaming items to rows, eh? :)

hehe, caught me. I can switch back, but "headings" doesn't really make sense without some matrix/table-ness structure imposed on the data anyways

paulirish · 2018-04-17T21:24:47Z

typings/lhr-lite.d.ts

+        valueType: 'url' | 'text' | 'link' | 'timespanMs';
+      }
+
+      export type AuditTableRow = WastedBytesDetailsRow | WastedTimeDetailsRow;


can u inline this into the two uses above?

brendankenny · 2018-04-17T21:25:16Z

typings/lhr-lite.d.ts

+      export interface AuditOpportunityDetails {
+        type: 'opportunity';
+        wastedMs: number
+        wastedBytes?: number


should we rename these totalWastedMs/totalWastedBytes? To make it clear what they are

Crazy idea. since these are opportunities, we could stay on the optimistic side.. something like.. "estimatedSavingsMs" ?

estimatedSavingsMs

I'm on board with that @patrickhulce

patrickhulce · 2018-04-17T21:23:19Z

typings/lhr-lite.d.ts

+      /** An object containing the top-level categories, their overall scores, and reference to member audits. */
+      categories?: Record<string, ResultLite.Category>;
+      /** Descriptions of the audit groups referenced by AuditRefs. */
+      categoryGroups?: Record<string, ResultLite.CategoryGroup>;


Did we decide of groups for this? I don't remember at this point 😆 or auditGroups aka "groups of related audits"

patrickhulce · 2018-04-17T21:24:00Z

typings/lhr-lite.d.ts

+      /** The version of Lighthouse with which these results were generated. */
+      lighthouseVersion: string;
+      /** An object containing the results of the audits. */
+      audits: Record<string, ResultLite.Audit>;


since we don't have any references to what auditId really is how do you feel about aliases like type AuditId = string?

patrickhulce · 2018-04-17T21:24:33Z

typings/lhr-lite.d.ts

+         * 'notApplicable': the audit turned out to not apply to the page. Ignore the score.
+         * 'manual': The audit exists only to tell you to review something yourself. Ignore the score.
+         */
+        scoreDisplayMode: 'binary' | 'numeric' | 'informative' | 'notApplicable' | 'manual';


I like it 👍

patrickhulce · 2018-04-17T21:26:33Z

typings/lhr-lite.d.ts

+      export interface AuditColumnHeading {
+        key: string;
+        label: string;
+        valueType: 'url' | 'text' | 'link' | 'timespanMs';


also have bytes and duration

duration -> timespanMs I thought? Also do we actually have text or links if it's just WastedBytesDetailsItem or WastedTimeDetailsItem?

patrickhulce · 2018-04-17T21:27:13Z

typings/lhr-lite.d.ts

+      }
+
+      export interface AuditTableDetails {
+        type: 'table';


do we actually have any of these that we want to expose? seems like we're only wanting to expose the opportunities which all use the table headings/rows types

brendankenny · 2018-04-17T21:37:09Z

oh yeah, a big one is explanation. Currently it's on details...should it just be on the audit result?

brendankenny · 2018-04-18T00:45:39Z

typings/lhr-lite.d.ts

+      /** An object containing the results of the audits. */
+      audits: Record<string, ResultLite.Audit>;
+      /** An object containing the top-level categories, their overall scores, and reference to member audits. */
+      categories?: Record<string, ResultLite.Category>;


should be required, even if an empty object (API responses should always have categories, so no need to make people jump through hoops checking for categories existence every time)

brendankenny · 2018-04-18T23:41:01Z

I think the last decision is categoryGroups/groups/auditGroups/etc? (I believe with the split on where the groups should live we decided on the status quo of leaving at the top level).

Any thoughts on that and last changes before I commit us to this? :)

patrickhulce · 2018-04-19T15:47:28Z

I think the last decision is categoryGroups/groups/auditGroups/etc? (I believe with the split on where the groups should live we decided on the status quo of leaving at the top level).
Any thoughts on that and last changes before I commit us to this? :)

Aw shoot sorry I forgot to submit my review!! Yes I have a thought. Do we even need these in LHR lite?
I realized they're a display-only concern and my favorite names for them are just groups/displayGroups. Seems like we can punt from LHR lite but maybe I'm missing something?

brendankenny · 2018-04-19T16:38:28Z

if it were groups vs displayGroups I'd tip slightly toward displayGroups.

I can't remember exactly why we added them to LHR-lite but I believe @paulirish thought it would be desired by consumers/trivial to add.

paulirish · 2018-04-19T19:43:17Z

okay i'm on board with leaving groups out of LHR-lite. we're not 100% on them and there's not a high priority reason to include them. (i think i started with "hey, might as well.")

metrics & opps are already distinguishable from details.type.
and manual audits are also distinguishable.
so that just leaves the grouping within a11y and seo.

The only one there that could be useful is seo-mobile, our mobile friendly group. but.. eh.

k let's boot groups.

patrickhulce · 2018-04-19T19:44:44Z

cool cool +1 to brendan's fav for displayGroups and displayGroupId for our real LHR :)

brendankenny · 2018-04-20T20:20:33Z

ready for final 🎖 💯

@vinamratasingal FYI on API return value

patrickhulce

I think this is great! I left 😬 everywhere I have a sinking feeling I'll regret that in ~6 months, but IMO this could ship today and I'm 👍

patrickhulce · 2018-04-20T20:22:05Z

typings/lhr-lite.d.ts

+        /** The overall score of the category, the weighted average of all its audits. */
+        score: number;
+        /** An array of references to all the audit members of this category. */
+        auditRefs: AuditRef[];


patrickhulce · 2018-04-20T20:22:14Z

typings/lhr-lite.d.ts

+        /** An array of references to all the audit members of this category. */
+        auditRefs: AuditRef[];
+        /** An optional description for manual audits within this category. */
+        manualDescription?: string;


patrickhulce · 2018-04-20T20:23:27Z

typings/lhr-lite.d.ts

+        /** A more detailed description that describes why the audit is important and links to Lighthouse documentation on the audit; markdown links supported. */
+        description: string;
+        /** The scored value determined by the audit, in the range `0-1`, or null if `scoreDisplayMode` indicates not scored. */
+        score: number | null;


FWIW I'd prefer score?: number

patrickhulce · 2018-04-20T20:26:06Z

typings/lhr-lite.d.ts

+        export interface MetricDetails {
+          type: 'metric';
+          /** The value of the metric expressed in milliseconds. */
+          timespanMs?: number;


I'm still 😬, but I think we mostly exhausted options. last-ditch effort anyone like ms timeMs timingMs more?

i consider timespanMs/ms/timingMs to be equally 😬 and i'd be fine with any of those.

brendankenny · 2018-04-21T02:24:48Z

@paulirish up to you now

core(lhr): lhr-lite proposed typings

332ddcf

devtools-bot added the waiting4reviewer label Apr 17, 2018

brendankenny commented Apr 17, 2018

View reviewed changes

brendankenny added 2 commits April 17, 2018 14:15

switcheroo

4c1c3a7

opportunities also table

8a405a3

paulirish requested changes Apr 17, 2018

View reviewed changes

brendankenny commented Apr 17, 2018

View reviewed changes

patrickhulce reviewed Apr 17, 2018

View reviewed changes

feedback

e20b98b

brendankenny commented Apr 18, 2018

View reviewed changes

always categories

2aaea82

vinamratasingal-zz mentioned this pull request Apr 18, 2018

Lighthouse 3.0 and I/O launches docs needed. google/WebFundamentals#6065

Closed

5 tasks

remove groups; add null as score option

102c585

brendankenny changed the title ~~[WIP] core(lhr): lhr-lite proposed types~~ core(lhr): lhr-lite type declaration Apr 20, 2018

patrickhulce approved these changes Apr 20, 2018

View reviewed changes

paulirish approved these changes Apr 20, 2018

View reviewed changes

paulirish mentioned this pull request Apr 20, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

brendankenny merged commit cf76f05 into master Apr 23, 2018

brendankenny deleted the lhrliteproposal branch April 23, 2018 18:39

paulirish removed the waiting4reviewer label Apr 30, 2018

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

core(lhr): lhr-lite type declaration (GoogleChrome#4983)

b4d8cd3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(lhr): lhr-lite type declaration #4983

core(lhr): lhr-lite type declaration #4983

brendankenny commented Apr 17, 2018

brendankenny Apr 17, 2018

patrickhulce Apr 17, 2018

paulirish Apr 17, 2018

brendankenny Apr 17, 2018

paulirish Apr 17, 2018

brendankenny Apr 17, 2018

paulirish Apr 17, 2018

brendankenny Apr 17, 2018

patrickhulce Apr 17, 2018

patrickhulce Apr 17, 2018

patrickhulce Apr 17, 2018

patrickhulce Apr 17, 2018

brendankenny Apr 17, 2018

patrickhulce Apr 17, 2018

brendankenny commented Apr 17, 2018

brendankenny Apr 18, 2018

brendankenny commented Apr 18, 2018

patrickhulce commented Apr 19, 2018

brendankenny commented Apr 19, 2018

paulirish commented Apr 19, 2018

patrickhulce commented Apr 19, 2018

brendankenny commented Apr 20, 2018

patrickhulce left a comment

patrickhulce Apr 20, 2018

patrickhulce Apr 20, 2018

patrickhulce Apr 20, 2018

patrickhulce Apr 20, 2018

paulirish Apr 20, 2018

brendankenny commented Apr 21, 2018

core(lhr): lhr-lite type declaration #4983

core(lhr): lhr-lite type declaration #4983

Conversation

brendankenny commented Apr 17, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 17, 2018

Choose a reason for hiding this comment

brendankenny commented Apr 18, 2018

patrickhulce commented Apr 19, 2018

brendankenny commented Apr 19, 2018

paulirish commented Apr 19, 2018

patrickhulce commented Apr 19, 2018

brendankenny commented Apr 20, 2018

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

Choose a reason for hiding this comment

brendankenny commented Apr 21, 2018