Skip to content
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.

Sign up for GitHub

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(tracehouse): allow missing FCP #9174

Merged
merged 6 commits into from
Jun 20, 2019
Merged

core(tracehouse): allow missing FCP #9174

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b4bdbb0
core(tracehouse): allow missing FCP
patrickhulce Jun 10, 2019
f1b007c
lint
patrickhulce Jun 10, 2019
f631271
add counter test
patrickhulce Jun 10, 2019
01e1ceb
manually construct traceOfTab
patrickhulce Jun 19, 2019
d7395ae
remove no fcp test
patrickhulce Jun 19, 2019
fc6c18c
last feedback
patrickhulce Jun 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions lighthouse-core/computed/trace-of-tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
*/
Expand All @@ -43,7 +36,20 @@ class TraceOfTab {
* @return {Promise<LH.Artifacts.TraceOfTab>}
*/
static async compute_(trace) {
return LHTraceProcessor.computeTraceOfTab(trace);
const traceOfTab = await LHTraceProcessor.computeTraceOfTab(trace);
const {timings, timestamps, firstContentfulPaintEvt} = traceOfTab;
const {firstContentfulPaint: firstContentfulPaintTiming} = timings;
const {firstContentfulPaint: firstContentfulPaintTs} = timestamps;
if (!firstContentfulPaintEvt || !firstContentfulPaintTiming || !firstContentfulPaintTs) {
throw new LHError(LHError.errors.NO_FCP);
}

return {
...traceOfTab,
firstContentfulPaintEvt,
timings: {...timings, firstContentfulPaint: firstContentfulPaintTiming},
timestamps: {...timestamps, firstContentfulPaint: firstContentfulPaintTs},
};
Copy link
Member

Choose a reason for hiding this comment

The 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)

}
}

Expand Down
22 changes: 8 additions & 14 deletions lighthouse-core/lib/tracehouse/trace-processor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Member

Choose a reason for hiding this comment

The 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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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?):/;
Expand All @@ -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}
*/
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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),
Expand Down
5 changes: 2 additions & 3 deletions lighthouse-core/test/lib/tracehouse/trace-processor-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,9 +436,8 @@ describe('TraceProcessor', () => {
.toThrowError('navigationStart');
});

it('throws on traces missing an FCP', () => {
expect(() => TraceProcessor.computeTraceOfTab(noFCPtrace))
.toThrowError('firstContentfulPaint');
it('does not throw on traces missing an FCP', () => {
expect(() => TraceProcessor.computeTraceOfTab(noFCPtrace)).not.toThrow();
});
});
});