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

feat(config): add extends lighthouse:full #2557

Merged
merged 1 commit into from
Jun 29, 2017
Merged

Conversation

patrickhulce
Copy link
Collaborator

last step to support #2518 and the forthcoming js coverage PR

split into several commits for easier viewing

  1. squashed version of feat(config): add support for audit blacklisting in isolation #2539 upon which this relies
  2. moves the default config to a new file called 'full-config.js'
  3. adds the necessary support to continue the ability to extend the default config in a non-breaking way (people who directly require the default.js file and filter themselves will be broken, but anyone using the LH filtering features will be fine)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

small meta discussion: thoughts about extending the other way, sort of like eslint: having lighthouse:default defining everything that goes in itself and then lighthouse:full extending the default and adding any non-default gatherers/audits to it?

I'm not sure if there's much to argue for that, though. The only advantage I can think of is that you'd be able to see all of what runs in a default LH run in one place instead of having to cross reference between two files. It might also make optimizing the default run easier, but I don't have much to back that up.

Any thoughts on advantages one way or the other?

@brendankenny
Copy link
Member

otherwise should be fine after updating for the config.js changes that landed

@patrickhulce
Copy link
Collaborator Author

I'm not sure if there's much to argue for that, though.

I do! That way would make the diff cleaner :) I'll give it a whirl

@patrickhulce patrickhulce changed the title refactor(config): move default to full-config feat(config): add extends lighthouse:full Jun 28, 2017
@patrickhulce
Copy link
Collaborator Author

this feels a lot better, PTAL :)

@paulirish
Copy link
Member

Yeah that was a great call. I'm happy with this.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM too

@patrickhulce patrickhulce merged commit 57bcf43 into master Jun 29, 2017
@patrickhulce patrickhulce deleted the default_full_config branch June 29, 2017 02:05
@brendankenny
Copy link
Member

if we start including a lot more audits in the repo that aren't included in the default config we may want to split Runner.getAuditList and Runner.getGathererList so things that won't be running in the extension/devtools don't get pulled into lighthouse-background.js

@patrickhulce
Copy link
Collaborator Author

if we start including a lot more audits in the repo that aren't included in the default config we may want to split Runner.getAuditList and Runner.getGathererList so things that won't be running in the extension/devtools don't get pulled into lighthouse-background.js

My intention is to add a checkbox in devtools to run these extra/slow audits so we'd still need them to be browserified, I actually was second guessing if not having them in the default config would get them in there at first haha :)

@brendankenny
Copy link
Member

oh, I was going to guess that at first, but isn't e.g. coverage kind of redundant in devtools when there's a better interface in other panels? I guess we could link between the report and those panels...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants