core(tracehouse): allow missing FCP (#9174)

GoogleChrome · Jun 20, 2019 · f1e1349 · f1e1349
1 parent 80a8058
commit f1e1349
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 25 deletions.
diff --git 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,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},
+    };
   }
 }
 

diff --git 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<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 */
+
 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),

diff --git a/lighthouse-core/test/lib/tracehouse/trace-processor-test.js b/lighthouse-core/test/lib/tracehouse/trace-processor-test.js
@@ -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();
     });
   });
 });