Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 4, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions lighthouse-core/config/config-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function used to be called cloneArrayWithPluginSafety (we used to call the custom audit/gatherer definition "plugin support") which became very misleading once we actually introduced plugin support that means something else. This is just more straightforward for what it was.

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,
Expand Down
66 changes: 7 additions & 59 deletions lighthouse-core/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -241,64 +246,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 {
/**
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand Down
108 changes: 108 additions & 0 deletions lighthouse-core/test/config/config-helpers-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/**
* @license Copyright 2020 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
* 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.
*/
'use strict';

const helpers = require('../../config/config-helpers.js');
const Gatherer = require('../../gather/gatherers/gatherer.js');
const UserTimingsAudit = require('../../audits/user-timings.js');

/* eslint-env jest */

describe('.deepClone', () => {
it('should clone things deeply', () => {
const input = {a: {b: {c: 1}}};
const output = helpers.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 = helpers.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', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Perhaps it's cleaner this way and it doesn't really matter, but it seems a bit unnecessary for these to be separate cases. We could just have a "preserves implementations" test that covers all three cases as they are all testing the same function and behavior.

const TimingGatherer = new Gatherer();
const input = {
passes: [{passName: 'defaultPass', gatherers: [TimingGatherer]}],
};

const output = helpers.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 = helpers.deepCloneConfigJson(input);
expect(output.artifacts[0].gatherer).toEqual(TimingGatherer);
});

it('should preserve audit implementations', () => {
const input = {
audits: [{implementation: UserTimingsAudit}],
};

const output = helpers.deepCloneConfigJson(input);
expect(output.audits[0].implementation).toEqual(UserTimingsAudit);
});
});


describe('.requireAudits', () => {
it('should expand audit short-hand', () => {
const result = helpers.requireAudits(['user-timings']);

expect(result).toEqual([{path: 'user-timings', options: {}, implementation: UserTimingsAudit}]);
});

it('should handle multiple audit definition styles', () => {
const result = helpers.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 = helpers.requireAudits(audits);
expect(merged).toMatchObject([
{path: 'user-timings', options: {}},
{path: 'is-on-https', options: {x: 2, y: 1}},
]);
});

it('throws for invalid auditDefns', () => {
expect(() => helpers.requireAudits([new Gatherer()])).toThrow(/Invalid Audit type/);
});
});
36 changes: 35 additions & 1 deletion types/config.d.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable strict */
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is anyone else having this problem in vscode where eslint is marking 100% of our .d.ts files as failing because of the strictness? started happening for me once we switched to the typescript parser in eslint

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup same. vscode's really unhappy with my .d.ts files.

/**
* @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
Expand All @@ -16,13 +17,36 @@ declare global {
export interface Json {
extends?: 'lighthouse:default' | string | boolean;
settings?: SharedFlagsSettings;
artifacts?: ArtifactJson[] | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah that's pleasant.

I faintly remember some other hack where we allowed something to be expressed in JSON instead of read from a filepath. Ah, looks like it was --extra-headers.

nevermind carry on!

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';
Expand All @@ -37,6 +61,11 @@ declare global {
gatherers?: GathererJson[];
}

export interface ArtifactJson {
id: string;
gatherer: GathererJson;
}

export type GathererJson = {
path: string;
options?: {};
Expand Down Expand Up @@ -86,6 +115,11 @@ declare global {
gatherers: GathererDefn[];
}

export interface ArtifactDefn {
id: string;
gatherer: GathererDefn;
}

export interface GathererDefn {
implementation?: typeof Gatherer;
instance: InstanceType<typeof Gatherer>;
Expand Down Expand Up @@ -125,4 +159,4 @@ declare global {
}

// empty export to keep file a module
export {}
export {};