-
Notifications
You must be signed in to change notification settings - Fork 9.6k
core(tracehouse): allow missing FCP #9174
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
Changes from all commits
b4bdbb0
f1b007c
f631271
01e1ceb
d7395ae
fc6c18c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,13 +19,6 @@ class LHTraceProcessor extends TraceProcessor { | |
| return new LHError(LHError.errors.NO_NAVSTART); | ||
| } | ||
|
|
||
| /** | ||
| * @return {Error} | ||
| */ | ||
| static createNoFirstContentfulPaintError() { | ||
| return new LHError(LHError.errors.NO_FCP); | ||
| } | ||
|
|
||
| /** | ||
| * @return {Error} | ||
| */ | ||
|
|
@@ -43,7 +36,29 @@ class TraceOfTab { | |
| * @return {Promise<LH.Artifacts.TraceOfTab>} | ||
| */ | ||
| static async compute_(trace) { | ||
| return LHTraceProcessor.computeTraceOfTab(trace); | ||
| // Trace of tab doesn't require FCP to exist, but all of LH requires it. | ||
| // We'll check that we got an FCP here and re-type accordingly so all of our consumers don't | ||
| // have to repeat this check. | ||
| const traceOfTab = await LHTraceProcessor.computeTraceOfTab(trace); | ||
| const {timings, timestamps, firstContentfulPaintEvt} = traceOfTab; | ||
| const {firstContentfulPaint: firstContentfulPaintTiming} = timings; | ||
| const {firstContentfulPaint: firstContentfulPaintTs} = timestamps; | ||
| if ( | ||
| !firstContentfulPaintEvt || | ||
| firstContentfulPaintTiming === undefined || | ||
| firstContentfulPaintTs === undefined | ||
| ) { | ||
| throw new LHError(LHError.errors.NO_FCP); | ||
| } | ||
|
|
||
| // We already know that `traceOfTab` is good to go at this point, but tsc doesn't yet. | ||
| // Help tsc out by reconstructing the object manually with the known defined values. | ||
| return { | ||
| ...traceOfTab, | ||
| firstContentfulPaintEvt, | ||
| timings: {...timings, firstContentfulPaint: firstContentfulPaintTiming}, | ||
| timestamps: {...timestamps, firstContentfulPaint: firstContentfulPaintTs}, | ||
| }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,9 @@ | |
| * 4. Return all those items in one handy bundle. | ||
| */ | ||
|
|
||
| /** @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 */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these types seem kind of terrible for downstream users, but I guess we can iterate on it in the future :)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. heh, yeah there will be lots of interesting problems to solve when we expose these types :) |
||
|
|
||
| const log = require('lighthouse-logger'); | ||
|
|
||
| const ACCEPTABLE_NAVIGATION_URL_REGEX = /^(chrome|https?):/; | ||
|
|
@@ -40,13 +43,6 @@ class TraceProcessor { | |
| return new Error('No navigationStart event found'); | ||
| } | ||
|
|
||
| /** | ||
| * @return {Error} | ||
| */ | ||
| static createNoFirstContentfulPaintError() { | ||
| return new Error('No firstContentfulPaint event found'); | ||
| } | ||
|
|
||
| /** | ||
| * @return {Error} | ||
| */ | ||
|
|
@@ -351,7 +347,7 @@ class TraceProcessor { | |
| * Finds key trace events, identifies main process/thread, and returns timings of trace events | ||
| * in milliseconds since navigation start in addition to the standard microsecond monotonic timestamps. | ||
| * @param {LH.Trace} trace | ||
| * @return {LH.Artifacts.TraceOfTab} | ||
| * @return {TraceOfTabWithoutFCP} | ||
| */ | ||
| static computeTraceOfTab(trace) { | ||
| // Parse the trace for our key events and sort them by timestamp. Note: sort | ||
|
|
@@ -380,7 +376,6 @@ class TraceProcessor { | |
| const firstContentfulPaint = frameEvents.find( | ||
| e => e.name === 'firstContentfulPaint' && e.ts > navigationStart.ts | ||
| ); | ||
| if (!firstContentfulPaint) throw this.createNoFirstContentfulPaintError(); | ||
|
|
||
| // fMP will follow at/after the FP | ||
| let firstMeaningfulPaint = frameEvents.find( | ||
|
|
@@ -424,27 +419,26 @@ class TraceProcessor { | |
|
|
||
| /** @param {{ts: number}=} event */ | ||
| const getTimestamp = (event) => event && event.ts; | ||
| /** @type {LH.Artifacts.TraceTimes} */ | ||
| /** @type {TraceTimesWithoutFCP} */ | ||
| const timestamps = { | ||
| navigationStart: navigationStart.ts, | ||
| firstPaint: getTimestamp(firstPaint), | ||
| firstContentfulPaint: firstContentfulPaint.ts, | ||
| firstContentfulPaint: getTimestamp(firstContentfulPaint), | ||
| firstMeaningfulPaint: getTimestamp(firstMeaningfulPaint), | ||
| traceEnd: fakeEndOfTraceEvt.ts, | ||
| load: getTimestamp(load), | ||
| domContentLoaded: getTimestamp(domContentLoaded), | ||
| }; | ||
|
|
||
|
|
||
| /** @param {number} ts */ | ||
| const getTiming = (ts) => (ts - navigationStart.ts) / 1000; | ||
| /** @param {number=} ts */ | ||
| const maybeGetTiming = (ts) => ts === undefined ? undefined : getTiming(ts); | ||
| /** @type {LH.Artifacts.TraceTimes} */ | ||
| /** @type {TraceTimesWithoutFCP} */ | ||
| const timings = { | ||
| navigationStart: 0, | ||
| firstPaint: maybeGetTiming(timestamps.firstPaint), | ||
| firstContentfulPaint: getTiming(timestamps.firstContentfulPaint), | ||
| firstContentfulPaint: maybeGetTiming(timestamps.firstContentfulPaint), | ||
| firstMeaningfulPaint: maybeGetTiming(timestamps.firstMeaningfulPaint), | ||
| traceEnd: getTiming(timestamps.traceEnd), | ||
| load: maybeGetTiming(timestamps.load), | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.