core(stack-packs): adjustments to comments and types (GoogleChrome#8169)

jwadhwani · Apr 12, 2019 · bb8689d · bb8689d
1 parent 8d5ca69
commit bb8689d
Show file tree

Hide file tree

Showing 21 changed files with 144 additions and 106 deletions.
diff --git a/build/build-bundle.js b/build/build-bundle.js
@@ -53,7 +53,7 @@ async function browserifyFile(entryPath, distPath) {
 
   bundle
     // Transform the fs.readFile etc into inline strings.
-    .transform('brfs', {global: true, parserOpts: {ecmaVersion: 9}})
+    .transform('brfs', {global: true, parserOpts: {ecmaVersion: 10}})
     // Strip everything out of package.json includes except for the version.
     .transform('package-json-versionify');
 
@@ -121,6 +121,7 @@ function minifyScript(filePath) {
     shouldPrintComment: () => false, // Don't include @license or @preserve comments either
     plugins: [
       'syntax-object-rest-spread',
+      'syntax-async-generators',
     ],
     // sourceMaps: 'both'
   };

diff --git a/lighthouse-core/audits/dobetterweb/js-libraries.js b/lighthouse-core/audits/dobetterweb/js-libraries.js
@@ -30,12 +30,12 @@ class JsLibrariesAudit extends Audit {
    * @return {LH.Audit.Product}
    */
   static audit(artifacts) {
-    const libDetails = (artifacts.Stacks || [])
+    const libDetails = artifacts.Stacks
       .filter(stack => stack.detector === 'js')
       .map(stack => ({
         name: stack.name,
-        version: stack.version || undefined, // null if not detected
-        npm: stack.npm || undefined, // ~70% of libs come with this field
+        version: stack.version,
+        npm: stack.npm,
       }));
 
     /** @type {LH.Audit.Details.Table['headings']} */

diff --git a/lighthouse-core/audits/dobetterweb/no-vulnerable-libraries.js b/lighthouse-core/audits/dobetterweb/no-vulnerable-libraries.js
@@ -60,8 +60,8 @@ class NoVulnerableLibrariesAudit extends Audit {
 
   /**
    * Attempts to normalize the version.
-   * @param {?string} version
-   * @return {?string}
+   * @param {string|undefined} version
+   * @return {string|undefined}
    */
   static normalizeVersion(version) {
     if (!version) return version;
@@ -78,7 +78,7 @@ class NoVulnerableLibrariesAudit extends Audit {
 
   /**
    * @param {string} normalizedVersion
-   * @param {{name: string, version: string, npm?: string}} lib
+   * @param {LH.Artifacts.DetectedStack} lib
    * @param {SnykDB} snykDB
    * @return {Array<Vulnerability>}
    */
@@ -135,7 +135,7 @@ class NoVulnerableLibrariesAudit extends Audit {
    * @return {LH.Audit.Product}
    */
   static audit(artifacts) {
-    const foundLibraries = (artifacts.Stacks || []).filter(stack => stack.detector === 'js');
+    const foundLibraries = artifacts.Stacks.filter(stack => stack.detector === 'js');
     const snykDB = NoVulnerableLibrariesAudit.snykDB;
 
     if (!foundLibraries.length) {

diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
@@ -7,7 +7,7 @@
 
 const log = require('lighthouse-logger');
 const manifestParser = require('../lib/manifest-parser.js');
-const stacksGatherer = require('../lib/gatherer-stacks.js');
+const stacksGatherer = require('../lib/stack-collector.js');
 const LHError = require('../lib/lh-error.js');
 const URL = require('../lib/url-shim.js');
 const NetworkRecorder = require('../lib/network-recorder.js');
@@ -412,7 +412,7 @@ class GatherRunner {
       NetworkUserAgent: '', // updated later
       BenchmarkIndex: 0, // updated later
       WebAppManifest: null, // updated later
-      Stacks: null, // updated later
+      Stacks: [], // updated later
       traces: {},
       devtoolsLogs: {},
       settings: options.settings,
@@ -480,12 +480,18 @@ class GatherRunner {
         }
         await GatherRunner.beforePass(passContext, gathererResults);
         await GatherRunner.pass(passContext, gathererResults);
+
         if (isFirstPass) {
+          // Fetch the manifest, if it exists. Currently must be fetched before gatherers' `afterPass`.
           baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext);
-          baseArtifacts.Stacks = await stacksGatherer(passContext);
         }
+
         const passData = await GatherRunner.afterPass(passContext, gathererResults);
 
+        if (isFirstPass) {
+          baseArtifacts.Stacks = await stacksGatherer(passContext);
+        }
+
         // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts.
         baseArtifacts.devtoolsLogs[passConfig.passName] = passData.devtoolsLog;
 

diff --git a/lighthouse-core/lib/gatherer-stacks.js → lighthouse-core/lib/stack-collector.js b/lighthouse-core/lib/gatherer-stacks.js → lighthouse-core/lib/stack-collector.js
@@ -15,45 +15,52 @@
 
 const fs = require('fs');
 const libDetectorSource = fs.readFileSync(
-  require.resolve('js-library-detector/library/libraries.js'),
-  'utf8'
-);
+  require.resolve('js-library-detector/library/libraries.js'), 'utf8');
+
+/** @typedef {false | {version: string|null}} JSLibraryDetectorTestResult */
+/**
+ * @typedef JSLibraryDetectorTest
+ * @property {string} icon Essentially an id, useful if no npm name is detected.
+ * @property {string} url
+ * @property {string|null} npm npm module name, if applicable to library.
+ * @property {function(Window): JSLibraryDetectorTestResult | Promise<JSLibraryDetectorTestResult>} test Returns false if library is not present, otherwise returns an object that contains the library version (set to null if the version is not detected).
+ */
 
 /**
  * @typedef JSLibrary
  * @property {string} name
- * @property {string} version
- * @property {string} npm
- * @property {string} iconName
+ * @property {string} icon
+ * @property {string|null} version
+ * @property {string|null} npm
  */
 
 /**
  * Obtains a list of detected JS libraries and their versions.
  */
 /* istanbul ignore next */
-function detectLibraries() {
+async function detectLibraries() {
   /** @type {JSLibrary[]} */
   const libraries = [];
 
   // d41d8cd98f00b204e9800998ecf8427e_ is a consistent prefix used by the detect libraries
   // see https://github.com/HTTPArchive/httparchive/issues/77#issuecomment-291320900
+  /** @type {Record<string, JSLibraryDetectorTest>} */
   // @ts-ignore - injected libDetectorSource var
-  Object.entries(d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests).forEach(
-    async ([name, lib]) => {
-      // eslint-disable-line max-len
-      try {
-        const result = await lib.test(window);
-        if (result) {
-          libraries.push({
-            name: name,
-            version: result.version,
-            npm: lib.npm,
-            iconName: lib.icon,
-          });
-        }
-      } catch (e) {}
-    }
-  );
+  const libraryDetectorTests = d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests; // eslint-disable-line 
+
+  for (const [name, lib] of Object.entries(libraryDetectorTests)) {
+    try {
+      const result = await lib.test(window);
+      if (result) {
+        libraries.push({
+          name: name,
+          icon: lib.icon,
+          version: result.version,
+          npm: lib.npm,
+        });
+      }
+    } catch (e) {}
+  }
 
   return libraries;
 }
@@ -62,23 +69,22 @@ function detectLibraries() {
  * @param {LH.Gatherer.PassContext} passContext
  * @return {Promise<LH.Artifacts['Stacks']>}
  */
-async function getStacks(passContext) {
+async function collectStacks(passContext) {
   const expression = `(function () {
     ${libDetectorSource};
     return (${detectLibraries.toString()}());
   })()`;
 
-  const jsLibraries = /** @type {JSLibrary[]} */ (await passContext.driver.evaluateAsync(
-    expression
-  ));
+  /** @type {JSLibrary[]} */
+  const jsLibraries = await passContext.driver.evaluateAsync(expression);
 
   return jsLibraries.map(lib => ({
     detector: /** @type {'js'} */ ('js'),
-    id: lib.npm || lib.iconName,
+    id: lib.npm || lib.icon,
     name: lib.name,
-    version: lib.version,
-    npm: lib.npm,
+    version: lib.version || undefined,
+    npm: lib.npm || undefined,
   }));
 }
 
-module.exports = getStacks;
+module.exports = collectStacks;
diff --git a/lighthouse-core/lib/stack-packs.js b/lighthouse-core/lib/stack-packs.js
@@ -6,42 +6,48 @@
 'use strict';
 
 const stackPacks = require('@lighthouse/stack-packs');
+const log = require('lighthouse-logger');
 
+/**
+ * Pairs consisting of a stack pack's ID and the set of stacks needed to be
+ * detected in a page to display that pack's advice.
+ * @type {Array<{packId: string, requiredStacks: Array<string>}>}
+ */
 const stackPacksToInclude = [{
   packId: 'wordpress',
   requiredStacks: ['js:wordpress'],
 }];
 
 /**
- * @param {LH.Artifacts} artifacts
+ * Returns all packs that match the stacks found in the page.
+ * @param {LH.Artifacts['Stacks']} pageStacks
  * @return {Array<LH.Result.StackPack>}
  */
-function getStackPacks(artifacts) {
+function getStackPacks(pageStacks) {
   /** @type {Array<LH.Result.StackPack>} */
   const packs = [];
 
-  if (artifacts.Stacks) {
-    for (const pageStack of artifacts.Stacks) {
-      const stackPackToIncl = stackPacksToInclude.find(stackPackToIncl =>
-        stackPackToIncl.requiredStacks.includes(`${pageStack.detector}:${pageStack.id}`));
-      if (!stackPackToIncl) {
-        continue;
-      }
-
-      // Grab the full pack definition
-      const matchedPack = stackPacks.find(pack => pack.id === stackPackToIncl.packId);
-      if (!matchedPack) {
-        // we couldn't find a pack that's in our inclusion list, this is weird.
-        continue;
-      }
-
-      packs.push({
-        id: matchedPack.id,
-        title: matchedPack.title,
-        iconDataURL: matchedPack.iconDataURL,
-        descriptions: matchedPack.descriptions,
-      });
+  for (const pageStack of pageStacks) {
+    const stackPackToIncl = stackPacksToInclude.find(stackPackToIncl =>
+      stackPackToIncl.requiredStacks.includes(`${pageStack.detector}:${pageStack.id}`));
+    if (!stackPackToIncl) {
+      continue;
     }
+
+    // Grab the full pack definition
+    const matchedPack = stackPacks.find(pack => pack.id === stackPackToIncl.packId);
+    if (!matchedPack) {
+      log.warn('StackPacks',
+        `'${stackPackToIncl.packId}' stack pack was matched but is not found in stack-packs lib`);
+      continue;
+    }
+
+    packs.push({
+      id: matchedPack.id,
+      title: matchedPack.title,
+      iconDataURL: matchedPack.iconDataURL,
+      descriptions: matchedPack.descriptions,
+    });
   }
 
   return packs;

diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js
@@ -148,7 +148,7 @@ class Runner {
           rendererFormattedStrings: i18n.getRendererFormattedStrings(settings.locale),
           icuMessagePaths: {},
         },
-        stackPacks: stackPacks.getStackPacks(artifacts),
+        stackPacks: stackPacks.getStackPacks(artifacts.Stacks),
       };
 
       // Replace ICU message references with localized strings; save replaced paths in lhr.

diff --git a/lighthouse-core/test/audits/dobetterweb/js-libraries-test.js b/lighthouse-core/test/audits/dobetterweb/js-libraries-test.js
@@ -21,19 +21,19 @@ describe('Returns detected front-end JavaScript libraries', () => {
     const auditResult2 = JsLibrariesAudit.audit({
       Stacks: [
         {detector: 'js', name: 'lib1', version: '3.10.1', npm: 'lib1'},
-        {detector: 'js', name: 'lib2', version: null, npm: 'lib2'},
+        {detector: 'js', name: 'lib2', version: undefined, npm: 'lib2'},
       ],
     });
     assert.equal(auditResult2.rawValue, true);
 
     // LOTS of frontend libs
     const auditResult3 = JsLibrariesAudit.audit({
       Stacks: [
-        {detector: 'js', name: 'React', version: null, npm: 'react'},
-        {detector: 'js', name: 'Polymer', version: null, npm: 'polymer-core'},
-        {detector: 'js', name: 'Preact', version: null, npm: 'preact'},
-        {detector: 'js', name: 'Angular', version: null, npm: 'angular'},
-        {detector: 'js', name: 'jQuery', version: null, npm: 'jquery'},
+        {detector: 'js', name: 'React', version: undefined, npm: 'react'},
+        {detector: 'js', name: 'Polymer', version: undefined, npm: 'polymer-core'},
+        {detector: 'js', name: 'Preact', version: undefined, npm: 'preact'},
+        {detector: 'js', name: 'Angular', version: undefined, npm: 'angular'},
+        {detector: 'js', name: 'jQuery', version: undefined, npm: 'jquery'},
       ],
     });
     assert.equal(auditResult3.rawValue, true);
@@ -43,7 +43,7 @@ describe('Returns detected front-end JavaScript libraries', () => {
     const auditResult = JsLibrariesAudit.audit({
       Stacks: [
         {detector: 'js', name: 'lib1', version: '3.10.1', npm: 'lib1'},
-        {detector: 'js', name: 'lib2', version: null, npm: 'lib2'},
+        {detector: 'js', name: 'lib2', version: undefined, npm: 'lib2'},
       ],
     });
     const expected = [

diff --git a/lighthouse-core/test/audits/dobetterweb/no-vulnerable-libraries-test.js b/lighthouse-core/test/audits/dobetterweb/no-vulnerable-libraries-test.js
@@ -13,7 +13,7 @@ const assert = require('assert');
 describe('Avoids front-end JavaScript libraries with known vulnerabilities', () => {
   describe('#normalizeVersion', () => {
     it('should leave valid and unsavable versions untouched', () => {
-      assert.equal(NoVulnerableLibrariesAudit.normalizeVersion(null), null);
+      assert.equal(NoVulnerableLibrariesAudit.normalizeVersion(undefined), undefined);
       assert.equal(NoVulnerableLibrariesAudit.normalizeVersion('52.1.13'), '52.1.13');
       assert.equal(NoVulnerableLibrariesAudit.normalizeVersion('52.1.13-rc.1'), '52.1.13-rc.1');
       assert.equal(NoVulnerableLibrariesAudit.normalizeVersion('c0ab71056b936'), 'c0ab71056b936');
@@ -31,7 +31,7 @@ describe('Avoids front-end JavaScript libraries with known vulnerabilities', ()
       Stacks: [
         {detector: 'js', name: 'lib1', version: '1.0.0', npm: 'lib1'},
         {detector: 'js', name: 'angular', version: '1.1.4', npm: 'angular'},
-        {detector: 'js', name: 'lib3', version: null, npm: 'lib3'},
+        {detector: 'js', name: 'lib3', version: undefined, npm: 'lib3'},
       ],
     });
     assert.equal(auditResult.rawValue, false);
@@ -89,7 +89,7 @@ Array [
     const auditResult = NoVulnerableLibrariesAudit.audit({
       Stacks: [
         {detector: 'js', name: 'lib1', version: '3.10.1', npm: 'lib1'},
-        {detector: 'js', name: 'lib2', version: null, npm: 'lib2'},
+        {detector: 'js', name: 'lib2', version: undefined, npm: 'lib2'},
       ],
     });
     assert.equal(auditResult.rawValue, true);

diff --git a/lighthouse-core/test/fixtures/artifacts/empty-artifacts/artifacts.json b/lighthouse-core/test/fixtures/artifacts/empty-artifacts/artifacts.json
@@ -2,5 +2,6 @@
   "URL": {
     "requestedUrl": "https://example.com/",
     "finalUrl": "https://example.com/"
-  }
+  },
+  "Stacks": []
 }
diff --git a/lighthouse-core/test/fixtures/artifacts/perflog/artifacts.json b/lighthouse-core/test/fixtures/artifacts/perflog/artifacts.json
@@ -9,6 +9,7 @@
     "requestedUrl": "https://www.reddit.com/r/nba",
     "finalUrl": "https://www.reddit.com/r/nba"
   },
+  "Stacks": [],
   "MetaElements": [],
   "ViewportDimensions": {
     "innerWidth": 412,

diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js
@@ -14,7 +14,7 @@ const Config = require('../../config/config');
 const unresolvedPerfLog = require('./../fixtures/unresolved-perflog.json');
 const NetworkRequest = require('../../lib/network-request.js');
 
-jest.mock('../../lib/gatherer-stacks.js', () => () => Promise.resolve([]));
+jest.mock('../../lib/stack-collector.js', () => () => Promise.resolve([]));
 
 class TestGatherer extends Gatherer {
   constructor() {

diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js
@@ -20,7 +20,7 @@ const LHError = require('../lib/lh-error.js');
 
 /* eslint-env jest */
 
-jest.mock('../lib/gatherer-stacks.js', () => () => Promise.resolve([]));
+jest.mock('../lib/stack-collector.js', () => () => Promise.resolve([]));
 
 describe('Runner', () => {
   /** @type {jest.Mock} */