Repository: GoogleChrome/lighthouse
Pull request #7038 (2019-01-17 18:25:38)

core: use async/await in evaluateAsync
no change in behavior.


Patch:
diff --git a/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js b/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
index c00c19a47..2ce5f806e 100644
--- a/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
+++ b/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
@@ -6,7 +6,6 @@
 'use strict';
 
 const assert = require('assert');
-// @ts-ignore - typed where used.
 const parseCacheControl = require('parse-cache-control');
 const Audit = require('../audit');
 const NetworkRequest = require('../../lib/network-request');
@@ -105,22 +104,14 @@ class CacheHeaders extends Audit {
   }
 
   /**
-   * Computes the user-specified cache lifetime, 0 if explicit no-cache policy is in effect, and null if not
-   * user-specified. See https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
-   *
+   * Return max-age if defined, otherwise expires header if defined, and null if not.
    * @param {Map<string, string>} headers
-   * @param {{'no-cache'?: boolean,'no-store'?: boolean, 'max-age'?: number}} cacheControl Follows the potential settings of cache-control, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
+   * @param {ReturnType<typeof parseCacheControl>} cacheControl
    * @return {?number}
    */
   static computeCacheLifetimeInSeconds(headers, cacheControl) {
-    if (cacheControl) {
-      // Cache-Control takes precendence over expires
-      if (cacheControl['no-cache'] || cacheControl['no-store']) return 0;
-      const maxAge = cacheControl['max-age'];
-      if (maxAge !== undefined && Number.isFinite(maxAge)) return Math.max(maxAge, 0);
-    } else if ((headers.get('pragma') || '').includes('no-cache')) {
-      // The HTTP/1.0 Pragma header can disable caching if cache-control is not set, see https://tools.ietf.org/html/rfc7234#section-5.4
-      return 0;
+    if (cacheControl && cacheControl['max-age'] !== undefined) {
+      return cacheControl['max-age'];
     }
 
     const expiresHeaders = headers.get('expires');
@@ -128,7 +119,7 @@ class CacheHeaders extends Audit {
       const expires = new Date(expiresHeaders).getTime();
       // Invalid expires values MUST be treated as already expired
       if (!expires) return 0;
-      return Math.max(0, Math.ceil((expires - Date.now()) / 1000));
+      return Math.ceil((expires - Date.now()) / 1000);
     }
 
     return null;
@@ -169,6 +160,31 @@ class CacheHeaders extends Audit {
     );
   }
 
+  /**
+   * Returns true if headers suggest a record should not be cached for a long time.
+   * @param {Map<string, string>} headers
+   * @param {ReturnType<typeof parseCacheControl>} cacheControl
+   * @returns {boolean}
+   */
+  static shouldSkipRecord(headers, cacheControl) {
+    // The HTTP/1.0 Pragma header can disable caching if cache-control is not set, see https://tools.ietf.org/html/rfc7234#section-5.4
+    if (!cacheControl && (headers.get('pragma') || '').includes('no-cache')) {
+      return true;
+    }
+
+    // Ignore assets where policy implies they should not be cached long periods
+    if (cacheControl &&
+      (
+        cacheControl['must-validate'] ||
+        cacheControl['no-cache'] ||
+        cacheControl['no-store'] ||
+        cacheControl['private'])) {
+      return true;
+    }
+
+    return false;
+  }
+
   /**
    * @param {LH.Artifacts} artifacts
    * @param {LH.Audit.Context} context
@@ -197,15 +213,20 @@ class CacheHeaders extends Audit {
         }
 
         const cacheControl = parseCacheControl(headers.get('cache-control'));
-        let cacheLifetimeInSeconds = CacheHeaders.computeCacheLifetimeInSeconds(
-          headers,
-          cacheControl
-        );
+        if (this.shouldSkipRecord(headers, cacheControl)) {
+          continue;
+        }
 
-        // Ignore assets with an explicit no-cache policy
-        if (cacheLifetimeInSeconds === 0) continue;
+        // Ignore if cacheLifetimeInSeconds is a nonpositive number.
+        let cacheLifetimeInSeconds = CacheHeaders.computeCacheLifetimeInSeconds(
+          headers, cacheControl);
+        if (cacheLifetimeInSeconds !== null &&
+          (!Number.isFinite(cacheLifetimeInSeconds) || cacheLifetimeInSeconds <= 0)) {
+          continue;
+        }
         cacheLifetimeInSeconds = cacheLifetimeInSeconds || 0;
 
+        // Ignore assets whose cache lifetime is already high enough
         const cacheHitProbability = CacheHeaders.getCacheHitProbability(cacheLifetimeInSeconds);
         if (cacheHitProbability > IGNORE_THRESHOLD_IN_PERCENT) continue;
 
@@ -218,7 +239,10 @@ class CacheHeaders extends Audit {
 
         results.push({
           url,
-          cacheControl,
+          // Include cacheControl in results, but cast as any so table types
+          // are happy. cacheControl is not shown in the table so this is OK.
+          // TODO(bckenny): fix DetailsItem
+          cacheControl: /** @type {any} */ (cacheControl),
           cacheLifetimeMs: cacheLifetimeInSeconds * 1000,
           cacheHitProbability,
           totalBytes,
diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js
index a831effb9..7118aae81 100644
--- a/lighthouse-core/gather/driver.js
+++ b/lighthouse-core/gather/driver.js
@@ -340,14 +340,9 @@ class Driver {
    * @param {{useIsolation?: boolean}=} options
    * @return {Promise<*>}
    */
-  evaluateAsync(expression, options = {}) {
-    // tsc won't convert {Promise<number>|Promise<undefined>}, so cast manually.
-    // https://github.com/Microsoft/TypeScript/issues/7294
-    /** @type {Promise<number|undefined>} */
-    const contextIdPromise = options.useIsolation ?
-        this._getOrCreateIsolatedContextId() :
-        Promise.resolve(undefined);
-    return contextIdPromise.then(contextId => this._evaluateInContext(expression, contextId));
+  async evaluateAsync(expression, options = {}) {
+    const contextId = options.useIsolation ? await this._getOrCreateIsolatedContextId() : undefined;
+    return this._evaluateInContext(expression, contextId);
   }
 
   /**
diff --git a/types/parse-cache-control/index.d.ts b/types/parse-cache-control/index.d.ts
new file mode 100644
index 000000000..e78a485b6
--- /dev/null
+++ b/types/parse-cache-control/index.d.ts
@@ -0,0 +1,19 @@
+/**
+ * @license Copyright 2019 Google Inc. All Rights Reserved.
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+
+declare module 'parse-cache-control' {
+  // Follows the potential settings of cache-control, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
+  interface CacheHeaders {
+    'max-age'?: number;
+    'must-validate'?: boolean;
+    'no-cache'?: boolean;
+    'no-store'?: boolean;
+    'private'?: boolean;
+  }
+
+  function ParseCacheControl(headers?: string): CacheHeaders | null;
+  export = ParseCacheControl;
+}


Test patch:
diff --git a/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js b/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js
index 46afd745b..a4fa6ac43 100644
--- a/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js
+++ b/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js
@@ -74,6 +74,22 @@ describe('Cache headers audit', () => {
     });
   });
 
+  it('ignores nonpositive and nonfinite max-age headers', () => {
+    const infinityMaxAgeStringValue = '1'.repeat(400);
+    assert.equal(Number.parseInt(infinityMaxAgeStringValue), Infinity);
+    const networkRecords = [
+      networkRecord({headers: {'cache-control': 'max-age=' + infinityMaxAgeStringValue}}),
+      networkRecord({headers: {'cache-control': 'max-age=-' + infinityMaxAgeStringValue}}),
+      networkRecord({headers: {'cache-control': 'max-age=-100'}}),
+    ];
+
+    const context = {options, computedCache: new Map()};
+    return CacheHeadersAudit.audit(getArtifacts(networkRecords), context).then(result => {
+      const items = result.details.items;
+      assert.equal(items.length, 0);
+    });
+  });
+
   it('detects low value expires headers', () => {
     const expiresIn = seconds => new Date(Date.now() + seconds * 1000).toGMTString();
     const closeEnough = (actual, exp) => assert.ok(Math.abs(actual - exp) <= 1, 'invalid expires');
@@ -103,11 +119,10 @@ describe('Cache headers audit', () => {
 
     const networkRecords = [
       networkRecord({headers: {
-        'cache-control': 'must-revalidate,max-age=3600',
+        'cache-control': 'no-transform,max-age=3600',
         'expires': expiresIn(86400),
       }}),
       networkRecord({headers: {
-        'cache-control': 'private,must-revalidate',
         'expires': expiresIn(86400),
       }}),
     ];
@@ -173,6 +188,21 @@ describe('Cache headers audit', () => {
     });
   });
 
+  it('ignores assets where policy implies they should not be cached long periods', () => {
+    const networkRecords = [
+      networkRecord({headers: {'cache-control': 'must-validate'}}),
+      networkRecord({headers: {'cache-control': 'no-cache'}}),
+      networkRecord({headers: {'cache-control': 'private'}}),
+    ];
+
+    const context = {options, computedCache: new Map()};
+    return CacheHeadersAudit.audit(getArtifacts(networkRecords), context).then(result => {
+      const items = result.extendedInfo.value.results;
+      assert.equal(result.score, 1);
+      assert.equal(items.length, 0);
+    });
+  });
+
   it('ignores potentially uncacheable records', () => {
     const networkRecords = [
       networkRecord({statusCode: 500}),