From b4bdbb03e1763c79dbbc285d7bef995a4e18b75e Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 10 Jun 2019 17:37:00 -0500 Subject: [PATCH 1/6] core(tracehouse): allow missing FCP --- lighthouse-core/computed/trace-of-tab.js | 3 ++- .../lib/tracehouse/trace-processor.js | 22 +++++++++++-------- .../lib/tracehouse/trace-processor-test.js | 2 +- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/lighthouse-core/computed/trace-of-tab.js b/lighthouse-core/computed/trace-of-tab.js index b8f572c89dd8..0cf5eb6bc4a8 100644 --- a/lighthouse-core/computed/trace-of-tab.js +++ b/lighthouse-core/computed/trace-of-tab.js @@ -43,7 +43,8 @@ class TraceOfTab { * @return {Promise} */ static async compute_(trace) { - return LHTraceProcessor.computeTraceOfTab(trace); + const traceOfTab = await LHTraceProcessor.computeTraceOfTab(trace, {throwOnNoFCP: true}); + return /** @type {LH.Artifacts.TraceOfTab} */ (traceOfTab); } } diff --git a/lighthouse-core/lib/tracehouse/trace-processor.js b/lighthouse-core/lib/tracehouse/trace-processor.js index 2bc4c6feeca6..4ece3fc4ac9c 100644 --- a/lighthouse-core/lib/tracehouse/trace-processor.js +++ b/lighthouse-core/lib/tracehouse/trace-processor.js @@ -16,6 +16,9 @@ * 4. Return all those items in one handy bundle. */ +/** @typedef {Omit & {firstContentfulPaint?: number}} TraceTimesWithoutFCP */ +/** @typedef {Omit & {timings: TraceTimesWithoutFCP, timestamps: TraceTimesWithoutFCP, firstContentfulPaintEvt?: LH.Artifacts.TraceOfTab['firstContentfulPaintEvt']}} TraceOfTabWithoutFCP */ + const log = require('lighthouse-logger'); const ACCEPTABLE_NAVIGATION_URL_REGEX = /^(chrome|https?):/; @@ -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 => { @@ -377,10 +382,10 @@ class TraceProcessor { const firstPaint = frameEvents.find(e => e.name === 'firstPaint' && e.ts > navigationStart.ts); // FCP will follow at/after the FP. Used in so many places we require it. - const firstContentfulPaint = frameEvents.find( + let 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( @@ -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), diff --git a/lighthouse-core/test/lib/tracehouse/trace-processor-test.js b/lighthouse-core/test/lib/tracehouse/trace-processor-test.js index 66e1204d0c1c..3a31d7237746 100644 --- a/lighthouse-core/test/lib/tracehouse/trace-processor-test.js +++ b/lighthouse-core/test/lib/tracehouse/trace-processor-test.js @@ -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'); }); }); From f1b007ccc4837072e07d6149583a8af641ec5287 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 10 Jun 2019 17:39:57 -0500 Subject: [PATCH 2/6] lint --- lighthouse-core/lib/tracehouse/trace-processor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/lib/tracehouse/trace-processor.js b/lighthouse-core/lib/tracehouse/trace-processor.js index 4ece3fc4ac9c..59fa917744cb 100644 --- a/lighthouse-core/lib/tracehouse/trace-processor.js +++ b/lighthouse-core/lib/tracehouse/trace-processor.js @@ -382,7 +382,7 @@ class TraceProcessor { const firstPaint = frameEvents.find(e => e.name === 'firstPaint' && e.ts > navigationStart.ts); // FCP will follow at/after the FP. Used in so many places we require it. - let firstContentfulPaint = frameEvents.find( + const firstContentfulPaint = frameEvents.find( e => e.name === 'firstContentfulPaint' && e.ts > navigationStart.ts ); if (!firstContentfulPaint && throwOnNoFCP) throw this.createNoFirstContentfulPaintError(); From f63127159e355bd5bc2ceafcb63c493e53a341dd Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 10 Jun 2019 17:45:51 -0500 Subject: [PATCH 3/6] add counter test --- lighthouse-core/test/lib/tracehouse/trace-processor-test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lighthouse-core/test/lib/tracehouse/trace-processor-test.js b/lighthouse-core/test/lib/tracehouse/trace-processor-test.js index 3a31d7237746..06b063abe331 100644 --- a/lighthouse-core/test/lib/tracehouse/trace-processor-test.js +++ b/lighthouse-core/test/lib/tracehouse/trace-processor-test.js @@ -436,6 +436,11 @@ describe('TraceProcessor', () => { .toThrowError('navigationStart'); }); + it('does not throw on traces missing an FCP by default', () => { + expect(() => TraceProcessor.computeTraceOfTab(noFCPtrace)) + .not.toThrowError('firstContentfulPaint'); + }); + it('throws on traces missing an FCP', () => { expect(() => TraceProcessor.computeTraceOfTab(noFCPtrace, {throwOnNoFCP: true})) .toThrowError('firstContentfulPaint'); From 01e1ceb3921a7aa235b38597f18a45d7184ee996 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 19 Jun 2019 09:56:34 -0500 Subject: [PATCH 4/6] manually construct traceOfTab --- lighthouse-core/computed/trace-of-tab.js | 23 +++++++++++-------- .../lib/tracehouse/trace-processor.js | 12 +--------- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/lighthouse-core/computed/trace-of-tab.js b/lighthouse-core/computed/trace-of-tab.js index 0cf5eb6bc4a8..22c7354ed5b0 100644 --- a/lighthouse-core/computed/trace-of-tab.js +++ b/lighthouse-core/computed/trace-of-tab.js @@ -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,8 +36,20 @@ class TraceOfTab { * @return {Promise} */ static async compute_(trace) { - const traceOfTab = await LHTraceProcessor.computeTraceOfTab(trace, {throwOnNoFCP: true}); - return /** @type {LH.Artifacts.TraceOfTab} */ (traceOfTab); + 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}, + }; } } diff --git a/lighthouse-core/lib/tracehouse/trace-processor.js b/lighthouse-core/lib/tracehouse/trace-processor.js index 59fa917744cb..49e798a581be 100644 --- a/lighthouse-core/lib/tracehouse/trace-processor.js +++ b/lighthouse-core/lib/tracehouse/trace-processor.js @@ -43,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} */ @@ -354,11 +347,9 @@ 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 - * @param {{throwOnNoFCP?: boolean}} options * @return {TraceOfTabWithoutFCP} */ - static computeTraceOfTab(trace, options = {}) { - const {throwOnNoFCP = false} = options; + static computeTraceOfTab(trace) { // 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 => { @@ -385,7 +376,6 @@ class TraceProcessor { const firstContentfulPaint = frameEvents.find( e => e.name === 'firstContentfulPaint' && e.ts > navigationStart.ts ); - if (!firstContentfulPaint && throwOnNoFCP) throw this.createNoFirstContentfulPaintError(); // fMP will follow at/after the FP let firstMeaningfulPaint = frameEvents.find( From d7395ae06705464e293aea47df0e345aea4877da Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 19 Jun 2019 09:58:55 -0500 Subject: [PATCH 5/6] remove no fcp test --- .../test/lib/tracehouse/trace-processor-test.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/lighthouse-core/test/lib/tracehouse/trace-processor-test.js b/lighthouse-core/test/lib/tracehouse/trace-processor-test.js index 06b063abe331..7c5dcc2bfa74 100644 --- a/lighthouse-core/test/lib/tracehouse/trace-processor-test.js +++ b/lighthouse-core/test/lib/tracehouse/trace-processor-test.js @@ -436,14 +436,8 @@ describe('TraceProcessor', () => { .toThrowError('navigationStart'); }); - it('does not throw on traces missing an FCP by default', () => { - expect(() => TraceProcessor.computeTraceOfTab(noFCPtrace)) - .not.toThrowError('firstContentfulPaint'); - }); - - it('throws on traces missing an FCP', () => { - expect(() => TraceProcessor.computeTraceOfTab(noFCPtrace, {throwOnNoFCP: true})) - .toThrowError('firstContentfulPaint'); + it('does not throw on traces missing an FCP', () => { + expect(() => TraceProcessor.computeTraceOfTab(noFCPtrace)).not.toThrow(); }); }); }); From fc6c18ca4a258c9d623668b147e3fa9069443810 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 19 Jun 2019 21:12:27 -0500 Subject: [PATCH 6/6] last feedback --- lighthouse-core/computed/trace-of-tab.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/computed/trace-of-tab.js b/lighthouse-core/computed/trace-of-tab.js index 22c7354ed5b0..6922f3810f09 100644 --- a/lighthouse-core/computed/trace-of-tab.js +++ b/lighthouse-core/computed/trace-of-tab.js @@ -36,14 +36,23 @@ class TraceOfTab { * @return {Promise} */ static async compute_(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 || !firstContentfulPaintTs) { + 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,