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 2 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
3 changes: 2 additions & 1 deletion lighthouse-core/computed/trace-of-tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ class TraceOfTab {
* @return {Promise<LH.Artifacts.TraceOfTab>}
*/
static async compute_(trace) {
return LHTraceProcessor.computeTraceOfTab(trace);
const traceOfTab = await LHTraceProcessor.computeTraceOfTab(trace, {throwOnNoFCP: true});
return /** @type {LH.Artifacts.TraceOfTab} */ (traceOfTab);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't that involve manually reconstructing every object that has fcp?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, ended up not being so bad I guess. done!

}
}

Expand Down
20 changes: 12 additions & 8 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 Down Expand Up @@ -351,9 +354,11 @@ 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}
* @param {{throwOnNoFCP?: boolean}} options
* @return {TraceOfTabWithoutFCP}
*/
static computeTraceOfTab(trace) {
static computeTraceOfTab(trace, options = {}) {
const {throwOnNoFCP = false} = options;
// Parse the trace for our key events and sort them by timestamp. Note: sort
// *must* be stable to keep events correctly nested.
const keyEvents = this._filteredStableSort(trace.traceEvents, e => {
Expand All @@ -380,7 +385,7 @@ class TraceProcessor {
const firstContentfulPaint = frameEvents.find(
e => e.name === 'firstContentfulPaint' && e.ts > navigationStart.ts
);
if (!firstContentfulPaint) throw this.createNoFirstContentfulPaintError();
if (!firstContentfulPaint && throwOnNoFCP) throw this.createNoFirstContentfulPaintError();

// fMP will follow at/after the FP
let firstMeaningfulPaint = frameEvents.find(
Expand Down Expand Up @@ -424,27 +429,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
2 changes: 1 addition & 1 deletion 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 @@ -437,7 +437,7 @@ describe('TraceProcessor', () => {
});

it('throws on traces missing an FCP', () => {
expect(() => TraceProcessor.computeTraceOfTab(noFCPtrace))
expect(() => TraceProcessor.computeTraceOfTab(noFCPtrace, {throwOnNoFCP: true}))
.toThrowError('firstContentfulPaint');
});
});
Expand Down