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): clean flags for settings #4960

Merged
merged 2 commits into from
Apr 13, 2018
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
20 changes: 18 additions & 2 deletions lighthouse-core/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,24 @@ function expandArtifacts(artifacts) {
return artifacts;
}

/**
* @param {LH.Flags=} flags
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a function description that this is dropping any flag not found on settings?

* @return {Partial<LH.Config.Settings>}
*/
function cleanFlagsForSettings(flags = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

semantically this is removedUndefinedProperties right?

could we call it that instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure I'm following you, this is looking at what properties are not on constants.defaultSettings and dropping them from flags regardless of their value.

the JSON.parse(JSON.stringify()) approach would address the specific undefined yargs case but not an underlying problem that having miscellaneous junk present that isn't part of settings throws off our isDeepEqual later

Copy link
Member

Choose a reason for hiding this comment

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

oh got it.

what properties are not on constants.defaultSettings

ahhh.. mmhmmm.

k lgtm

const settings = {};
for (const key of Object.keys(flags)) {
if (typeof constants.defaultSettings[key] !== 'undefined') {
settings[key] = flags[key];
}
}

return settings;
}

function merge(base, extension) {
if (typeof base === 'undefined') {
// If the default value doesn't exist or is explicitly null, defer to the extending value
if (typeof base === 'undefined' || base === null) {
return extension;
} else if (Array.isArray(extension)) {
if (!Array.isArray(base)) throw new TypeError(`Expected array but got ${typeof base}`);
Expand Down Expand Up @@ -330,7 +346,7 @@ class Config {
configJSON.passes = Config.expandGathererShorthandAndMergeOptions(configJSON.passes);

// Override any applicable settings with CLI flags
configJSON.settings = merge(configJSON.settings || {}, flags || {});
configJSON.settings = merge(configJSON.settings || {}, cleanFlagsForSettings(flags));

// Generate a limited config if specified
if (Array.isArray(configJSON.settings.onlyCategories) ||
Expand Down
14 changes: 14 additions & 0 deletions lighthouse-core/config/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,24 @@ const throttling = {
},
};

/** @type {LH.Config.Settings} */
Copy link
Member

Choose a reason for hiding this comment

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

👍

const defaultSettings = {
maxWaitForLoad: 45 * 1000,
throttlingMethod: 'devtools',
throttling: throttling.mobile3G,
auditMode: false,
gatherMode: false,
disableStorageReset: false,
disableDeviceEmulation: false,

// the following settings have no defaults but we still want ensure that `key in settings`
// in config will work in a typechecked way
blockedUrlPatterns: null,
additionalTraceCategories: null,
extraHeaders: null,
onlyAudits: null,
onlyCategories: null,
skipAudits: null,
};

const defaultPassConfig = {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,7 @@ class Driver {
}

/**
* @param {{additionalTraceCategories?: string}=} settings
* @param {{additionalTraceCategories?: string|null}=} settings
* @return {Promise<void>}
*/
beginTrace(settings) {
Expand Down Expand Up @@ -1019,7 +1019,7 @@ class Driver {
}

/**
* @param {LH.Crdp.Network.Headers=} headers key/value pairs of HTTP Headers.
* @param {LH.Crdp.Network.Headers|null} headers key/value pairs of HTTP Headers.
* @return {Promise<void>}
*/
async setExtraHTTPHeaders(headers) {
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/test/config/config-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,12 @@ describe('Config', () => {
});
});

it('cleans up flags for settings', () => {
const config = new Config({extends: true}, {nonsense: 1, foo: 2, throttlingMethod: 'provided'});
assert.equal(config.settings.throttlingMethod, 'provided');
assert.ok(config.settings.nonsense === undefined, 'did not cleanup settings');
});

it('extends the full config', () => {
class CustomAudit extends Audit {
static get meta() {
Expand Down
14 changes: 7 additions & 7 deletions typings/config.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ declare global {
settings?: SettingsJson;
passes?: PassJson[];
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

someone's got a diff trailing space editor setting it seems :)

Copy link
Member

Choose a reason for hiding this comment

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

ah, damn, eslint isn't checking .ts files, obvs. I'll find the setting :P


export interface SettingsJson extends SharedFlagsSettings {
extraHeaders?: Crdp.Network.Headers;
extraHeaders?: Crdp.Network.Headers | null;
}

export interface PassJson {
passName: string;
recordTrace?: boolean;
Expand All @@ -41,7 +41,7 @@ declare global {
blankDuration?: number;
gatherers: GathererJson[];
}

export type GathererJson = {
path: string;
options?: {};
Expand All @@ -52,14 +52,14 @@ declare global {
instance: InstanceType<typeof Gatherer>;
options?: {};
} | string;

// TODO(bckenny): we likely don't want to require all these
export type Settings = Required<SettingsJson>;

export interface Pass extends Required<PassJson> {
gatherers: GathererDefn[];
}

export interface GathererDefn {
implementation: typeof Gatherer;
instance: InstanceType<typeof Gatherer>;
Expand Down
10 changes: 5 additions & 5 deletions typings/externs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,17 @@ declare global {

interface SharedFlagsSettings {
maxWaitForLoad?: number;
blockedUrlPatterns?: string[];
additionalTraceCategories?: string;
blockedUrlPatterns?: string[] | null;
additionalTraceCategories?: string | null;
auditMode?: boolean | string;
gatherMode?: boolean | string;
disableStorageReset?: boolean;
disableDeviceEmulation?: boolean;
throttlingMethod?: 'devtools'|'simulate'|'provided';
throttling?: ThrottlingSettings;
onlyAudits?: string[];
onlyCategories?: string[];
skipAudits?: string[];
onlyAudits?: string[] | null;
onlyCategories?: string[] | null;
skipAudits?: string[] | null;
}

export interface Flags extends SharedFlagsSettings {
Expand Down