-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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): add type checking to asset-saver #4949
Conversation
metadata?: { | ||
'cpu-family'?: number; | ||
}; | ||
[futureProps: string]: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this added? afaik there aren't any other toplevel items in a trace than these two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this added?
we support outputting arbitrary keys in the trace object in saveTrace()
and logAssets()
, which seems worth keeping?
But then tsc needs to know there are other properties to loop over in the first place, so it needs to be either ts-ignored in place or a generic "every other property name" entry needs to be added here.
So...what do you think we should do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok. well that's fine. can you just leave a comment that this is for our flexible traceWriter method?
export interface Trace { | ||
traceEvents: TraceEvent[]; | ||
metadata?: { | ||
'cpu-family'?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious why you called out this but not the rest..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we support saving metadata
in asset-saver
, so we want to put it in there, but we don't really use it anywhere and I don't know what's actually required in there vs optional. However if we just put a metadata?: {}
it treats it as any
. Putting a single property triggers excess property checking, so could signal "hey, add some types if you want to use this". Could also set to never
or something for the same effect...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol ok
lighthouse-core/lib/asset-saver.js
Outdated
mkdirp.sync(basePath); | ||
rimraf.sync(`${basePath}/*${traceSuffix}`); | ||
rimraf.sync(`${basePath}/${artifactsFilename}`); | ||
|
||
// We don't want to mutate the artifacts as provided | ||
artifacts = Object.assign({}, artifacts); | ||
const {traces, devtoolsLogs, ...restArtifacts} = artifacts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So hot. 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a-ok by me
let promise = Promise.all(Object.keys(traces).map(passName => { | ||
return saveTrace(traces[passName], `${basePath}/${passName}${traceSuffix}`); | ||
})); | ||
for (const [passName, trace] of Object.entries(traces)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh this is so beautiful in comparison ❤️
object spread support is currently blocked (and making the tests fail) by brfs. I've filed an upstream bug but maybe I'll back out the spread line for now |
lgtm |
another step on the way to typing the LHR pipeline, but was self contained enough that I split it out for easier review. Adds types to
asset-saver.js
and a few places changes due to that touch, like e.g. making a type alias forLH.DevtoolsLog
(instead of (Array<LH.Protocol.RawEventMessage>
:).asset-saver.js
is mostly the same except for indenting, so probably easier to review with ?w=1.With the use of object spread, I moved our minimum node engine to 8.9 (the first Node 8 LTS release) and bumped our eslint to support what it calls
ecmaVersion
2018.