core(tsc): fix ImageUsage artifact type and gather bug (#5113)

GoogleChrome · May 4, 2018 · 91aa8d2 · 91aa8d2
1 parent af87207
commit 91aa8d2
Show file tree

Hide file tree

Showing 7 changed files with 62 additions and 43 deletions.
diff --git a/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js b/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js
@@ -110,12 +110,18 @@ module.exports = [
         },
       },
       'uses-responsive-images': {
+        displayValue: [
+          'Potential savings of %d\xa0KB',
+          75,
+        ],
         extendedInfo: {
           value: {
             wastedKb: '>50',
-            results: {
-              length: 3,
-            },
+            results: [
+              {wastedPercent: '<60'},
+              {wastedPercent: '<60'},
+              {wastedPercent: '<60'},
+            ],
           },
         },
       },

diff --git a/lighthouse-core/audits/byte-efficiency/offscreen-images.js b/lighthouse-core/audits/byte-efficiency/offscreen-images.js
@@ -19,6 +19,8 @@ const ALLOWABLE_OFFSCREEN_Y = 200;
 const IGNORE_THRESHOLD_IN_BYTES = 2048;
 const IGNORE_THRESHOLD_IN_PERCENT = 75;
 
+/** @typedef {{url: string, requestStartTime: number, totalBytes: number, wastedBytes: number, wastedPercent: number}} WasteResult */
+
 class OffscreenImages extends ByteEfficiencyAudit {
   /**
    * @return {LH.Audit.Meta}
@@ -38,7 +40,7 @@ class OffscreenImages extends ByteEfficiencyAudit {
   }
 
   /**
-   * @param {ClientRect} imageRect
+   * @param {{top: number, bottom: number, left: number, right: number}} imageRect
    * @param {{innerWidth: number, innerHeight: number}} viewportDimensions
    * @return {number}
    */
@@ -55,11 +57,15 @@ class OffscreenImages extends ByteEfficiencyAudit {
   }
 
   /**
-   * @param {!Object} image
+   * @param {LH.Artifacts.SingleImageUsage} image
    * @param {{innerWidth: number, innerHeight: number}} viewportDimensions
-   * @return {?Object}
+   * @return {null|Error|WasteResult}
    */
   static computeWaste(image, viewportDimensions) {
+    if (!image.networkRecord) {
+      return null;
+    }
+
     const url = URL.elideDataURI(image.src);
     const totalPixels = image.clientWidth * image.clientHeight;
     const visiblePixels = this.computeVisiblePixels(image.clientRect, viewportDimensions);
@@ -111,11 +117,11 @@ class OffscreenImages extends ByteEfficiencyAudit {
     /** @type {string|undefined} */
     let debugString;
     const resultsMap = images.reduce((results, image) => {
-      if (!image.networkRecord) {
+      const processed = OffscreenImages.computeWaste(image, viewportDimensions);
+      if (processed === null) {
         return results;
       }
 
-      const processed = OffscreenImages.computeWaste(image, viewportDimensions);
       if (processed instanceof Error) {
         debugString = processed.message;
         // @ts-ignore TODO(bckenny): Sentry type checking
@@ -130,7 +136,7 @@ class OffscreenImages extends ByteEfficiencyAudit {
       }
 
       return results;
-    }, new Map());
+    }, /** @type {Map<string, WasteResult>} */ (new Map()));
 
     const settings = context.settings;
     return artifacts.requestFirstCPUIdle({trace, devtoolsLog, settings}).then(firstInteractive => {

diff --git a/lighthouse-core/audits/byte-efficiency/uses-responsive-images.js b/lighthouse-core/audits/byte-efficiency/uses-responsive-images.js
@@ -43,6 +43,11 @@ class UsesResponsiveImages extends ByteEfficiencyAudit {
    * @return {null|Error|LH.Audit.ByteEfficiencyResult};
    */
   static computeWaste(image, DPR) {
+    // Nothing can be done without network info.
+    if (!image.networkRecord) {
+      return null;
+    }
+
     const url = URL.elideDataURI(image.src);
     const actualPixels = image.naturalWidth * image.naturalHeight;
     const usedPixels = image.clientWidth * image.clientHeight * Math.pow(DPR, 2);

diff --git a/lighthouse-core/audits/image-aspect-ratio.js b/lighthouse-core/audits/image-aspect-ratio.js
@@ -9,14 +9,15 @@
  *   audit will list all images that don't match with their display size
  *   aspect ratio.
  */
-// @ts-nocheck - TODO(bckenny): fix optional width/height in ImageUsage artifact
 'use strict';
 
 const Audit = require('./audit');
 
 const URL = require('../lib/url-shim');
 const THRESHOLD = 0.05;
 
+/** @typedef {Required<LH.Artifacts.SingleImageUsage>} WellDefinedImage */
+
 class ImageAspectRatio extends Audit {
   /**
    * @return {LH.Audit.Meta}
@@ -32,7 +33,7 @@ class ImageAspectRatio extends Audit {
   }
 
   /**
-   * @param {Required<LH.Artifacts.SingleImageUsage>} image
+   * @param {WellDefinedImage} image
    * @return {Error|{url: string, displayedAspectRatio: string, actualAspectRatio: string, doRatiosMatch: boolean}}
    */
   static computeAspectRatios(image) {
@@ -76,7 +77,7 @@ class ImageAspectRatio extends Audit {
         image.height &&
         !image.usesObjectFit;
     }).forEach(image => {
-      const wellDefinedImage = /** @type {Required<LH.Artifacts.SingleImageUsage>} */ (image);
+      const wellDefinedImage = /** @type {WellDefinedImage} */ (image);
       const processed = ImageAspectRatio.computeAspectRatios(wellDefinedImage);
       if (processed instanceof Error) {
         debugString = processed.message;

diff --git a/lighthouse-core/gather/gatherers/image-usage.js b/lighthouse-core/gather/gatherers/image-usage.js
@@ -15,9 +15,7 @@ const Driver = require('../driver.js'); // eslint-disable-line no-unused-vars
 
 /* global window, getElementsInDocument, Image */
 
-/** @typedef {MakeOptional<LH.Artifacts.SingleImageUsage, 'networkRecord'>} ImageSizeInfo */
-
-/** @return {Array<ImageSizeInfo>} */
+/** @return {Array<LH.Artifacts.SingleImageUsage>} */
 /* istanbul ignore next */
 function collectImageElementInfo() {
   /** @param {Element} element */
@@ -39,7 +37,7 @@ function collectImageElementInfo() {
     return element.localName === 'img';
   }));
 
-  /** @type {Array<ImageSizeInfo>} */
+  /** @type {Array<LH.Artifacts.SingleImageUsage>} */
   const htmlImages = allImageElements.map(element => {
     const computedStyle = window.getComputedStyle(element);
     return {
@@ -97,7 +95,7 @@ function collectImageElementInfo() {
     });
 
     return images;
-  }, /** @type {Array<ImageSizeInfo>} */ ([]));
+  }, /** @type {Array<LH.Artifacts.SingleImageUsage>} */ ([]));
 
   return htmlImages.concat(cssImages);
 }
@@ -125,8 +123,8 @@ function determineNaturalSize(url) {
 class ImageUsage extends Gatherer {
   /**
    * @param {Driver} driver
-   * @param {ImageSizeInfo} element
-   * @return {Promise<ImageSizeInfo>}
+   * @param {LH.Artifacts.SingleImageUsage} element
+   * @return {Promise<LH.Artifacts.SingleImageUsage>}
    */
   async fetchElementWithSizeInformation(driver, element) {
     const url = JSON.stringify(element.src);
@@ -145,7 +143,7 @@ class ImageUsage extends Gatherer {
    * @param {LH.Gatherer.LoadData} loadData
    * @return {Promise<LH.Artifacts['ImageUsage']>}
    */
-  afterPass(passContext, loadData) {
+  async afterPass(passContext, loadData) {
     const driver = passContext.driver;
     const indexedNetworkRecords = loadData.networkRecords.reduce((map, record) => {
       if (/^image/.test(record._mimeType) && record.finished) {
@@ -167,27 +165,25 @@ class ImageUsage extends Gatherer {
       return (${collectImageElementInfo.toString()})();
     })()`;
 
-    /** @type {Promise<Array<ImageSizeInfo>>} */
-    const evaluatePromise = driver.evaluateAsync(expression);
-    return evaluatePromise.then(elements => {
-      return elements.reduce((promise, element) => {
-        return promise.then(collector => {
-          // Images within `picture` behave strangely and natural size information isn't accurate,
-          // CSS images have no natural size information at all.
-          // Try to get the actual size if we can.
-          const elementPromise = (element.isPicture || element.isCss) && element.networkRecord ?
-              this.fetchElementWithSizeInformation(driver, element) :
-              Promise.resolve(element);
-
-          return elementPromise.then(element => {
-            // link up the image with its network record
-            element.networkRecord = indexedNetworkRecords[element.src];
-            collector.push(/** @type {LH.Artifacts.SingleImageUsage} */ (element));
-            return collector;
-          });
-        });
-      }, Promise.resolve(/** @type {LH.Artifacts['ImageUsage']} */ ([])));
-    });
+    /** @type {Array<LH.Artifacts.SingleImageUsage>} */
+    const elements = await driver.evaluateAsync(expression);
+
+    const imageUsage = [];
+    for (let element of elements) {
+      // link up the image with its network record
+      element.networkRecord = indexedNetworkRecords[element.src];
+
+      // Images within `picture` behave strangely and natural size information isn't accurate,
+      // CSS images have no natural size information at all. Try to get the actual size if we can.
+      // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image.
+      if ((element.isPicture || element.isCss) && element.networkRecord) {
+        element = await this.fetchElementWithSizeInformation(driver, element);
+      }
+
+      imageUsage.push(element);
+    }
+
+    return imageUsage;
   }
 }
 

diff --git a/typings/artifacts.d.ts b/typings/artifacts.d.ts
@@ -221,14 +221,16 @@ declare global {
           left: number;
           right: number;
         };
-        networkRecord: {
+        networkRecord?: {
           url: string;
           resourceSize: number;
           startTime: number;
           endTime: number;
           responseReceivedTime: number;
           mimeType: string;
-        }
+        };
+        width?: number;
+        height?: number;
       }
 
       export interface OptimizedImage {

diff --git a/typings/externs.d.ts b/typings/externs.d.ts
@@ -22,6 +22,9 @@ declare global {
     [P in K]+?: T[P]
   }
 
+  /** Remove properties K from T. */
+  type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>;
+
   /** Obtain the type of the first parameter of a function. */
   type FirstParamType<T extends (arg1: any, ...args: any[]) => any> =
     T extends (arg1: infer P, ...args: any[]) => any ? P : any;