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.

Sign up for GitHub

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

Jump to bottom

feat: allow extension of default config #1731

Merged
merged 3 commits into from
Feb 17, 2017
Merged

feat: allow extension of default config #1731

merged 3 commits into from
Feb 17, 2017

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Feb 16, 2017 •
edited
Loading

would address part of #1730, it's rather out there so comments and thoughts welcome on other possible approaches

usage

{
  extends: true,
  passes: [
    {passName: "defaultPass", gatherers: [MyCustomGatherer]
  ],
  audits: [MyCustomAudit]
}

@patrickhulce patrickhulce mentioned this pull request Feb 16, 2017
Merged
@ebidel
Copy link
Contributor

ebidel commented Feb 16, 2017

This looks useful. I dreamed of similar things in #887. Our single gigantic config is being unwieldy. This will also help consumers of LH more easily create custom configs.

ebidel
ebidel reviewed Feb 16, 2017
View reviewed changes
lighthouse-core/config/config.js Outdated
return value;
} else {
return extension;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ditch else and just return extension;?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

lighthouse-core/config/config.js Outdated
@@ -262,6 +284,12 @@ class Config {
if (Array.isArray(inputConfig.audits)) {
configJSON.audits = Array.from(inputConfig.audits);
}

// Extend the default config is specified
Copy link
Contributor

Choose a reason for hiding this comment

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

// Extend the default config if specified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

lighthouse-core/config/config.js Outdated
@@ -280,6 +308,27 @@ class Config {
validatePasses(configJSON.passes, this._audits, this._configDir);
}

static extendConfigJSON(baseJSON, extendJSON) {
Copy link
Contributor

Choose a reason for hiding this comment

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

might want to add jsdocs, even though it's fairly obvious what everything is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@ebidel
Copy link
Contributor

ebidel commented Feb 16, 2017

Can you add documentation to the README? In the "creating custom configs" section.

paulirish
paulirish reviewed Feb 16, 2017
View reviewed changes
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

yup generally like this approach and think itll be more flexible than passing options from CLI for everything. nice.

lighthouse-core/config/config.js Outdated
@@ -233,6 +233,27 @@ function expandArtifacts(artifacts) {
return artifacts;
}

function merge(value, extension) {
Copy link
Member

Choose a reason for hiding this comment

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

how about s/value/base/. communicates a little better which is being mutated, and in line with your extendConfigJSON below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

lighthouse-core/config/config.js Outdated
if (!Array.isArray(value)) throw new TypeError(`Expected array but got ${typeof value}`);
return value.concat(extension);
} else if (typeof extension === 'object') {
if (typeof value !== 'object') throw new TypeError(`Expected array but got ${typeof value}`);
Copy link
Member

Choose a reason for hiding this comment

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

array => object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

woops, done

lighthouse-core/config/config.js Outdated
delete extendJSON.passes;
}

Object.keys(extendJSON).forEach(key => {
Copy link
Member

Choose a reason for hiding this comment

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

couldn't these three lines be replaced with merge(baseJSON, extendJSON) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep! artifact of earlier implementation, done

@patrickhulce patrickhulce changed the title Proposal: Allow extension of default config feat: allow extension of default config Feb 16, 2017
@patrickhulce
Copy link
Collaborator Author

@ebidel added a readme entry PTAL :)

@paulirish paulirish merged commit 50a68ab into master Feb 17, 2017
@paulirish paulirish deleted the config_extenstions branch February 17, 2017 05:06
@patrickhulce patrickhulce mentioned this pull request Feb 17, 2017
Closed
3 tasks
@brendankenny brendankenny mentioned this pull request Mar 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebidel ebidel ebidel approved these changes

@paulirish paulirish paulirish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@patrickhulce @ebidel @paulirish