core(tracehouse): allow missing FCP #9174
Conversation
| static async compute_(trace) { | ||
| return LHTraceProcessor.computeTraceOfTab(trace); | ||
| const traceOfTab = await LHTraceProcessor.computeTraceOfTab(trace, {throwOnNoFCP: true}); | ||
| return /** @type {LH.Artifacts.TraceOfTab} */ (traceOfTab); |
There was a problem hiding this comment.
I really don't like this, but I was blanking on a way to do this without casting and/or manually reconstructing the entire object.
any ideas @brendankenny ?
There was a problem hiding this comment.
what if we didn't throw in trace-processor.js at all and only throw NO_FCP out here if it's not on the return value?
There was a problem hiding this comment.
doesn't that involve manually reconstructing every object that has fcp?
There was a problem hiding this comment.
doesn't that involve manually reconstructing every object that has fcp?
yeah, I can't think of a way around it, tsc won't track the validations on all three existence checks into the return value :/
It would allow simplifying trace-processor.js (no argument, no fcp check in there, etc), though, and it would keep the fcp checks and cast in the same place in this file.
There was a problem hiding this comment.
yeah, ended up not being so bad I guess. done!
|
ping 🏓 |
because the computed artifact already has a test for this? (I mean for overall testing, obviously the |
brendankenny
left a comment
brendankenny
left a comment
There was a problem hiding this comment.
nice! last nits but LGTM
| */ | ||
|
|
||
| /** @typedef {Omit<LH.Artifacts.TraceTimes, 'firstContentfulPaint'> & {firstContentfulPaint?: number}} TraceTimesWithoutFCP */ | ||
| /** @typedef {Omit<LH.Artifacts.TraceOfTab, 'firstContentfulPaintEvt'|'timings'|'timestamps'> & {timings: TraceTimesWithoutFCP, timestamps: TraceTimesWithoutFCP, firstContentfulPaintEvt?: LH.Artifacts.TraceOfTab['firstContentfulPaintEvt']}} TraceOfTabWithoutFCP */ |
There was a problem hiding this comment.
these types seem kind of terrible for downstream users, but I guess we can iterate on it in the future :)
There was a problem hiding this comment.
heh, yeah there will be lots of interesting problems to solve when we expose these types :)
| firstContentfulPaintEvt, | ||
| timings: {...timings, firstContentfulPaint: firstContentfulPaintTiming}, | ||
| timestamps: {...timestamps, firstContentfulPaint: firstContentfulPaintTs}, | ||
| }; |
There was a problem hiding this comment.
ha, I don't entirely trust tsc with object spreading + composition, but any change I make here gives the right error, so good job compiler! (and @patrickhulce)
Yes, that commit just removed the test from |
3 participants
Summary
One of the only changes made to tracium that needs to be ported back, part of the larger plan specified in #8956.
Related Issues/PRs
#8956