core(tsc): begin clean up of audit details types #7154
Conversation
| @@ -23,7 +23,6 @@ const superLongURL = | |||
| 'https://example.com/thisIsASuperLongURLThatWillTriggerFilenameTruncationWhichWeWantToTest.js'; | |||
| const DETAILS = { | |||
| type: 'criticalrequestchain', | |||
| header: {type: 'text', text: 'CRC Header'}, | |||
There was a problem hiding this comment.
only executable code change (though in a test). We haven't done anything with a header field in I don't know how long, though, so might as well delete it today :)
| @@ -70,49 +70,10 @@ declare global { | |||
| /** An explanation of audit-related issues encountered on the test page. */ | |||
| explanation?: string; | |||
| /** Extra information provided by some types of audits. */ | |||
| details?: Audit.MetricDetails | Audit.OpportunityDetails; | |||
| details?: never; | |||
There was a problem hiding this comment.
I'm going to start deleting lhr-lite-d.ts since its use didn't pan out, so starting by snipping off everything starting here
| // export interface Table {} | ||
| // export interface SomeKindOfHiddenThing | ||
|
|
||
| // Contents of details below here |
There was a problem hiding this comment.
maybe we want a different namespace here, not sure. These are things that are only inside details (e.g. table headings, table items, but expanding soon with Table and MultiCheck populated).
There was a problem hiding this comment.
LH.Audit.Details.Details ;)
I'd personally be in favor of naming convention over a different namespace. they categorically they definitely belong under "details". maybe we return to *Details suffix if these get out of hand?
There was a problem hiding this comment.
I'd personally be in favor of naming convention over a different namespace. they categorically they definitely belong under "details". maybe we return to
*Detailssuffix if these get out of hand?
yeah, I'm ok with living with this for now, just trying to think of ways to shrink the amount of thinking one has to do. Part of this was motivated by reviewing #6999 and seeing some of our existing types like DetailsRendererDetailsJSON and DetailsRendererNodeDetailsJSON and going "wat" :)
| scale: number; | ||
| items: { | ||
| timing: number; | ||
| timestamp: number; |
There was a problem hiding this comment.
wanna throw a comment on these bad boys for filmstrip? we were inconsistent when we added the full page screenshot so documenting what they represent seems like a decent idea :)
There was a problem hiding this comment.
wanna throw a comment on these bad boys for filmstrip
done
types/audit-details.d.ts
Outdated
| // TODO(bckenny) | ||
| // export interface MultiCheck {} | ||
| // export interface Table {} | ||
| // export interface SomeKindOfHiddenThing |
There was a problem hiding this comment.
these intentionally left in here?
these would be like a foreign language to me if I were ever tasked with following up maybe we could flesh out the comment a tad? :D
There was a problem hiding this comment.
these intentionally left in here?
I shouldn't leave those there. Deleting :)
| // export interface Table {} | ||
| // export interface SomeKindOfHiddenThing | ||
|
|
||
| // Contents of details below here |
There was a problem hiding this comment.
LH.Audit.Details.Details ;)
I'd personally be in favor of naming convention over a different namespace. they categorically they definitely belong under "details". maybe we return to *Details suffix if these get out of hand?
wading through #6999 makes it clear that our
detailstype situation is approaching untenable :)This starts us off, unifying several top-level details types under
LH.Audit.Details. There are no functional changes, just types, so this is just describing what we're already doing in a slightly different way (hopefully better).This is the cleanest set of things I could find to do before starting to need code changes, so we'll build from here :)
Happy to bikeshed on names