core(config): refactor config cloning for fraggle rock (#11759)

lighthouse-core/config/config-helpers.js

@@ -255,7 +255,75 @@ function resolveModule(moduleIdentifier, configDir, category) {
        ${relativePath}`);
 }
 
+/**
+ * Many objects in the config can be an object whose properties are not serializable.
+ * We use a shallow clone for these objects instead.
+ * Any value that isn't an object will not be cloned.
+ *
+ * @template T
+ * @param {T} item
+ * @return {T}
+ */
+function shallowClone(item) {
+  if (typeof item === 'object') {
+    // Return copy of instance and prototype chain (in case item is instantiated class).
+    return Object.assign(
+      Object.create(
+        Object.getPrototypeOf(item)
+      ),
+      item
+    );
+  }
+
+  return item;
+}
+
+/**
+ * // TODO(bckenny): could adopt "jsonified" type to ensure T will survive JSON
+ * round trip: https://github.com/Microsoft/TypeScript/issues/21838
+ * @template T
+ * @param {T} json
+ * @return {T}
+ */
+function deepClone(json) {
+  return JSON.parse(JSON.stringify(json));
+}
+
+/**
+ * Deep clone a ConfigJson, copying over any "live" gatherer or audit that
+ * wouldn't make the JSON round trip.
+ * @param {LH.Config.Json} json
+ * @return {LH.Config.Json}
+ */
+function deepCloneConfigJson(json) {
+  const cloned = deepClone(json);
+
+  // Copy arrays that could contain non-serializable properties to allow for programmatic
+  // injection of audit and gatherer implementations.
+  if (Array.isArray(cloned.passes) && Array.isArray(json.passes)) {
+    for (let i = 0; i < cloned.passes.length; i++) {
+      const pass = cloned.passes[i];
+      pass.gatherers = (json.passes[i].gatherers || []).map(gatherer => shallowClone(gatherer));
+    }
+  }
+
+  if (Array.isArray(json.audits)) {
+    cloned.audits = json.audits.map(audit => shallowClone(audit));
+  }
+
+  if (Array.isArray(json.artifacts)) {
+    cloned.artifacts = json.artifacts.map(artifact => ({
+      ...artifact,
+      gatherer: shallowClone(artifact.gatherer),
+    }));
+  }
+
+  return cloned;
+}
+
 module.exports = {
+  deepClone,
+  deepCloneConfigJson,
   mergeOptionsOfItems,
   requireAudits,
   resolveModule,

lighthouse-core/config/config.js

@@ -16,7 +16,12 @@ const path = require('path');
 const Runner = require('../runner.js');
 const ConfigPlugin = require('./config-plugin.js');
 const Budget = require('./budget.js');
-const {requireAudits, resolveModule} = require('./config-helpers.js');
+const {
+  requireAudits,
+  resolveModule,
+  deepClone,
+  deepCloneConfigJson,
+} = require('./config-helpers.js');
 
 /** @typedef {typeof import('../gather/gatherers/gatherer.js')} GathererConstructor */
 /** @typedef {InstanceType<GathererConstructor>} Gatherer */
@@ -282,64 +287,7 @@ function _merge(base, extension, overwriteArrays = false) {
 const merge = _merge;
 
 /**
- * @template T
- * @param {Array<T>} array
- * @return {Array<T>}
- */
-function cloneArrayWithPluginSafety(array) {
-  return array.map(item => {
-    if (typeof item === 'object') {
-      // Return copy of instance and prototype chain (in case item is instantiated class).
-      return Object.assign(
-        Object.create(
-          Object.getPrototypeOf(item)
-        ),
-        item
-      );
-    }
-
-    return item;
-  });
-}
-
-/**
- * // TODO(bckenny): could adopt "jsonified" type to ensure T will survive JSON
- * round trip: https://github.com/Microsoft/TypeScript/issues/21838
- * @template T
- * @param {T} json
- * @return {T}
- */
-function deepClone(json) {
-  return JSON.parse(JSON.stringify(json));
-}
-
-/**
- * Deep clone a ConfigJson, copying over any "live" gatherer or audit that
- * wouldn't make the JSON round trip.
- * @param {LH.Config.Json} json
- * @return {LH.Config.Json}
- */
-function deepCloneConfigJson(json) {
-  const cloned = deepClone(json);
-
-  // Copy arrays that could contain plugins to allow for programmatic
-  // injection of plugins.
-  if (Array.isArray(cloned.passes) && Array.isArray(json.passes)) {
-    for (let i = 0; i < cloned.passes.length; i++) {
-      const pass = cloned.passes[i];
-      pass.gatherers = cloneArrayWithPluginSafety(json.passes[i].gatherers || []);
-    }
-  }
-
-  if (Array.isArray(json.audits)) {
-    cloned.audits = cloneArrayWithPluginSafety(json.audits);
-  }
-
-  return cloned;
-}
-
-/**
- * @implements {LH.Config.Json}
+ * @implements {LH.Config.Config}
  */
 class Config {
   /**

lighthouse-core/runner.js

@@ -22,7 +22,7 @@ const generateReport = require('./report/report-generator.js').generateReport;
 const LHError = require('./lib/lh-error.js');
 
 /** @typedef {import('./gather/connections/connection.js')} Connection */
-/** @typedef {import('./config/config.js')} Config */
+/** @typedef {LH.Config.Config} Config */
 
 class Runner {
   /**

lighthouse-core/test/config/config-helpers-test.js

@@ -7,14 +7,112 @@
 
 /* eslint-env jest */
 
-const {resolveModule} = require('../../config/config-helpers.js');
-const assert = require('assert').strict;
 const path = require('path');
+const {deepClone, deepCloneConfigJson, requireAudits, resolveModule} =
+  require('../../config/config-helpers.js');
+const Gatherer = require('../../gather/gatherers/gatherer.js');
+const UserTimingsAudit = require('../../audits/user-timings.js');
 
 jest.mock('process', () => ({
   cwd: () => jest.fn(),
 }));
 
+describe('.deepClone', () => {
+  it('should clone things deeply', () => {
+    const input = {a: {b: {c: 1}}};
+    const output = deepClone(input);
+    expect(output).not.toBe(input);
+    expect(output).toEqual(input);
+    output.a.b.c = 2;
+    expect(input.a.b.c).toEqual(1);
+  });
+});
+
+describe('.deepCloneConfigJson', () => {
+  it('should clone a config deeply', () => {
+    const TimingGatherer = new Gatherer();
+    const input = {
+      artifacts: [{id: 'Timing', gatherer: TimingGatherer}],
+      passes: [{passName: 'defaultPass', gatherers: []}],
+      audits: [{path: 'user-timings'}],
+      categories: {random: {auditRefs: [{id: 'user-timings'}]}},
+    };
+
+    const output = deepCloneConfigJson(input);
+    expect(output).not.toBe(input);
+    expect(output).toEqual(input);
+    output.artifacts[0].id = 'NewName';
+    output.passes[0].passName = 'newName';
+    output.audits[0].path = 'new-audit';
+    output.categories.random.auditRefs[0].id = 'new-audit';
+    expect(input.artifacts[0].id).toEqual('Timing');
+    expect(input.passes[0].passName).toEqual('defaultPass');
+    expect(input.audits[0].path).toEqual('user-timings');
+    expect(input.categories.random.auditRefs[0].id).toEqual('user-timings');
+  });
+
+  it('should preserve gatherer implementations in passes', () => {
+    const TimingGatherer = new Gatherer();
+    const input = {
+      passes: [{passName: 'defaultPass', gatherers: [TimingGatherer]}],
+    };
+
+    const output = deepCloneConfigJson(input);
+    expect(output.passes[0].gatherers[0]).toEqual(TimingGatherer);
+  });
+
+  it('should preserve gatherer implementations in artifacts', () => {
+    const TimingGatherer = new Gatherer();
+    const input = {
+      artifacts: [{id: 'Timing', gatherer: TimingGatherer}],
+    };
+
+    const output = deepCloneConfigJson(input);
+    expect(output.artifacts[0].gatherer).toEqual(TimingGatherer);
+  });
+
+  it('should preserve audit implementations', () => {
+    const input = {
+      audits: [{implementation: UserTimingsAudit}],
+    };
+
+    const output = deepCloneConfigJson(input);
+    expect(output.audits[0].implementation).toEqual(UserTimingsAudit);
+  });
+});
+
+
+describe('.requireAudits', () => {
+  it('should expand audit short-hand', () => {
+    const result = requireAudits(['user-timings']);
+
+    expect(result).toEqual([{path: 'user-timings', options: {}, implementation: UserTimingsAudit}]);
+  });
+
+  it('should handle multiple audit definition styles', () => {
+    const result = requireAudits(['user-timings', {implementation: UserTimingsAudit}]);
+
+    expect(result).toMatchObject([{path: 'user-timings'}, {implementation: UserTimingsAudit}]);
+  });
+
+  it('should merge audit options', () => {
+    const audits = [
+      'user-timings',
+      {path: 'is-on-https', options: {x: 1, y: 1}},
+      {path: 'is-on-https', options: {x: 2}},
+    ];
+    const merged = requireAudits(audits);
+    expect(merged).toMatchObject([
+      {path: 'user-timings', options: {}},
+      {path: 'is-on-https', options: {x: 2, y: 1}},
+    ]);
+  });
+
+  it('throws for invalid auditDefns', () => {
+    expect(() => requireAudits([new Gatherer()])).toThrow(/Invalid Audit type/);
+  });
+});
+
 describe('resolveModule', () => {
   const configFixturePath = path.resolve(__dirname, '../fixtures/config');
 
@@ -25,21 +123,21 @@ describe('resolveModule', () => {
   it('lighthouse and plugins are installed in the same path', () => {
     const pluginName = 'chrome-launcher';
     const pathToPlugin = resolveModule(pluginName, null, 'plugin');
-    assert.equal(pathToPlugin, require.resolve(pluginName));
+    expect(pathToPlugin).toEqual(require.resolve(pluginName));
   });
 
   describe('plugin paths to a file', () => {
     it('relative to the current working directory', () => {
       const pluginName = 'lighthouse-plugin-config-helper';
       const pathToPlugin = resolveModule(pluginName, null, 'plugin');
-      assert.equal(pathToPlugin, require.resolve(path.resolve(configFixturePath, pluginName)));
+      expect(pathToPlugin).toEqual(require.resolve(path.resolve(configFixturePath, pluginName)));
     });
 
     it('relative to the config path', () => {
       process.cwd = jest.fn(() => path.resolve(configFixturePath, '../'));
       const pluginName = 'lighthouse-plugin-config-helper';
       const pathToPlugin = resolveModule(pluginName, configFixturePath, 'plugin');
-      assert.equal(pathToPlugin, require.resolve(path.resolve(configFixturePath, pluginName)));
+      expect(pathToPlugin).toEqual(require.resolve(path.resolve(configFixturePath, pluginName)));
     });
   });
 
@@ -56,7 +154,7 @@ describe('resolveModule', () => {
 
       const pathToPlugin = resolveModule(pluginName, null, 'plugin');
 
-      assert.equal(pathToPlugin, require.resolve(pluginName, {paths: [pluginDir]}));
+      expect(pathToPlugin).toEqual(require.resolve(pluginName, {paths: [pluginDir]}));
     });
 
     // working directory/
@@ -71,7 +169,7 @@ describe('resolveModule', () => {
 
       const pathToPlugin = resolveModule(pluginName, configDirectory, 'plugin');
 
-      assert.equal(pathToPlugin, require.resolve(pluginName, {paths: [configDirectory]}));
+      expect(pathToPlugin).toEqual(require.resolve(pluginName, {paths: [configDirectory]}));
     });
   });
 });

types/config.d.ts

@@ -1,3 +1,4 @@
+/* eslint-disable strict */
 /**
  * @license Copyright 2018 The Lighthouse Authors. 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
@@ -19,13 +20,36 @@ declare global {
       export interface Json {
         extends?: 'lighthouse:default' | string;
         settings?: SharedFlagsSettings;
+        artifacts?: ArtifactJson[] | null;
         passes?: PassJson[] | null;
         audits?: Config.AuditJson[] | null;
         categories?: Record<string, CategoryJson> | null;
         groups?: Record<string, Config.GroupJson> | null;
         plugins?: Array<string>,
       }
 
+      /**
+       * The normalized and fully resolved config.
+       */
+      export interface Config {
+        settings: Settings;
+        passes: Pass[] | null;
+        audits: AuditDefn[] | null;
+        categories: Record<string, Category> | null;
+        groups: Record<string, Group> | null;
+      }
+
+      /**
+       * The normalized and fully resolved Fraggle Rock config.
+       */
+      export interface FRConfig {
+        settings: Settings;
+        artifacts: ArtifactDefn[] | null;
+        audits: AuditDefn[] | null;
+        categories: Record<string, Category> | null;
+        groups: Record<string, Group> | null;
+      }
+
       export interface PassJson {
         passName: string;
         loadFailureMode?: 'fatal'|'warn'|'ignore';
@@ -40,6 +64,11 @@ declare global {
         gatherers?: GathererJson[];
       }
 
+      export interface ArtifactJson {
+        id: string;
+        gatherer: GathererJson;
+      }
+
       export type GathererJson = {
         path: string;
         options?: {};
@@ -91,6 +120,11 @@ declare global {
         gatherers: GathererDefn[];
       }
 
+      export interface ArtifactDefn {
+        id: string;
+        gatherer: GathererDefn;
+      }
+
       export interface GathererDefn {
         implementation?: ClassOf<Gatherer.GathererInstance>;
         instance: Gatherer.GathererInstance;