core(config): add support for audit/gatherer options #4394

patrickhulce · 2018-01-30T22:25:17Z

big change coming in! closes #4206

introduces support for audit and gatherer options while maintaining backcompat with existing config options, internally all audits and gatherers are mapped to the new format

additionally, the very poorly named options input to gatherers I'm now calling context as it more roughly captures what's being passed in, also we avoid options.options :)

feedback very very welcome while it's still early please! :D

Usage - before

module.exports = {
  passes: [{
    gatherers: [
      'builtin-gatherer',
      class MyGatherer extends Gatherer { },
  }],
  audits: [
    'builtin-audit',
    class MyAudit extends Audit { },
  ]
}

Usage - after

module.exports = {
  passes: [{
    gatherers: [
      'builtin-gatherer', // legacy
      {path: 'seo/other-gatherer', options: {foo: 1}}, // new

      class MyGatherer extends Gatherer { }, // legacy
      {implementation: class MyGatherer {}}, // new
      {instance: new (class MyGatherer extends Gatherer { })()}, // new
  }],
  audits: [
    'builtin-audit',
    {path: 'byte-efficiency/other-audit', options: {foo: 1}},
    class MyAudit extends Audit { },
  ]
}

wardpeet · 2018-01-31T21:18:33Z

lighthouse-core/config/config.js

  }

-  const auditIds = audits.map(audit => audit.meta.name);
+  const auditIds = audits.map(audit => audit.implementation.meta.name);


I find implementation such a weird name I wanted to call it instance but I see below that it's indeed the implementation but still find it weird 😛 but maybe that's because i'm no native speaker.

wardpeet · 2018-01-31T21:35:43Z

I really like this PR! not much to say about it. I haven't tested any of it but the approach looks good. I only think we need to document the option parameter a bit. We don't want it to be used as an escape hatch to smuggle custom settings in.

paulirish

Thanks for all the renames. It's great to have these things called more accurately.

A few questions below but first.. This is correct yah?

implementation: the class of a gatherer/audit
instance: an instance of that class/implementation
defn: whatever is resolved into the implementation.
- Can you show me what one looks like? a gathererDefn and an auditDefn?

paulirish · 2018-02-06T00:47:12Z

lighthouse-core/gather/gather-runner.js

+          return gathererDefn;
+        }
+
+        if (typeof gathererDefn.beforePass === 'function') {


this is for backwards compat? if so can you add a comment with the case this covers?

paulirish · 2018-02-06T00:52:43Z

lighthouse-core/gather/gather-runner.js

+    return gatherers.reduce((chain, gathererDefn) => {
      return chain.then(_ => {
-        const artifactPromise = Promise.resolve().then(_ => gatherer.pass(options));
+        const gatherer = gathererDefn.instance;


feels like pass/beforePass shouldn't have to do this resolving on their own. can we provide instantiated gatherers instead of the config.gatherers or something?

resolved in person, outcome: gather-runner will still need access to .instance and .options but will be oblivious to everything else

paulirish · 2018-02-06T01:00:49Z

lighthouse-core/gather/gather-runner.js

-        return new GathererClass();
+        const gathererPath = typeof gathererDefn === 'string' ? gathererDefn : gathererDefn.path;
+        const GathererClass = GatherRunner.getGathererClass(
+          gathererDefn.implementation || gathererPath, rootPath);


should we move all this into config.js? seems like it spiritually belongs over there.

There's also a lot of cases (6+?) being served by these lines:

lighthouse/lighthouse-core/gather/gather-runner.js

Lines 505 to 516 in c2652e6

// If this is already instantiated, don't do anything else.

if (gathererDefn.instance) {

return gathererDefn;

}

if (typeof gathererDefn.beforePass === 'function') {

return {instance: gathererDefn, options: {}};

}

const gathererPath = typeof gathererDefn === 'string' ? gathererDefn : gathererDefn.path;

const GathererClass = GatherRunner.getGathererClass(

gathererDefn.implementation || gathererPath, rootPath);

I'm hoping by moving it into config, we're going to end up with something simpler. Open to ideas.

yeah just lots of test changes 😢 I'm on it though

paulirish · 2018-02-06T01:21:31Z

lighthouse-core/gather/gather-runner.js

      return chain.then(_ => {
-        const artifactPromise = Promise.resolve().then(_ => gatherer.pass(options));
+        const gatherer = gathererDefn.instance;
+        context.options = gathererDefn.options || {};


at the very least let's comment that this is terrible. :)

I'll create a new context so it won't be terrible :)

paulirish · 2018-02-06T01:40:49Z

lighthouse-core/gather/gather-runner.js

    }
  }

  static instantiateGatherers(passes, rootPath) {


let's do instantiateGatherers back in config so by the time gatherrunner.run is called, everything has an instance.
this'll require updating some tests. sorry 'bout that.

…t_gatherer_options

paulirish

this is gtg, though I think we should land it after we cut the 2.9.1. sgty?

paulirish · 2018-02-09T16:18:26Z

lighthouse-core/gather/gather-runner.js

+        const gatherer = gathererDefn.instance;
+        // Abuse the passContext to pass through gatherer options
+        passContext.options = gathererDefn.options || {};
+        const artifactPromise = Promise.resolve().then(_ => gatherer.beforePass(passContext));


wdyt about a followup PR to rename options in all the gatherers to passContext?

patrickhulce · 2018-02-09T18:43:42Z

this is gtg, though I think we should land it after we cut the 2.9.1. sgty?

yeah good plan

patrickhulce added 3 commits January 29, 2018 17:58

core(config): add support for audit/gatherer options

8deb9f0

add tests and rename options -> context

415e9e4

more tests

c2652e6

patrickhulce requested review from kdzwinel, paulirish and wardpeet January 30, 2018 22:25

devtools-bot added the waiting4reviewer label Jan 30, 2018

wardpeet reviewed Jan 31, 2018

View reviewed changes

paulirish reviewed Feb 6, 2018

View reviewed changes

patrickhulce added 4 commits February 7, 2018 14:29

Merge branch 'master' of github.com:GoogleChrome/lighthouse into audi…

6322be8

…t_gatherer_options

move over tests

293c038

add more comments

b848614

add abusive comments warnings

a5ebdfc

paulirish approved these changes Feb 9, 2018

View reviewed changes

Merge branch 'master' into audit_gatherer_options

8594874

patrickhulce merged commit 5ea1de0 into master Feb 14, 2018

patrickhulce deleted the audit_gatherer_options branch February 14, 2018 18:59

patrickhulce mentioned this pull request Feb 20, 2018

extension: fix issue where gatherers cannot be found #4592

Merged

paulirish removed the waiting4reviewer label Mar 6, 2018

patrickhulce mentioned this pull request Dec 1, 2020

core(config): remove gatherer options support #11743

Merged

	// If this is already instantiated, don't do anything else.
	if (gathererDefn.instance) {
	return gathererDefn;
	}

	if (typeof gathererDefn.beforePass === 'function') {
	return {instance: gathererDefn, options: {}};
	}

	const gathererPath = typeof gathererDefn === 'string' ? gathererDefn : gathererDefn.path;
	const GathererClass = GatherRunner.getGathererClass(
	gathererDefn.implementation \|\| gathererPath, rootPath);

core(config): add support for audit/gatherer options #4394

core(config): add support for audit/gatherer options #4394

Uh oh!

Conversation

patrickhulce commented Jan 30, 2018 • edited by paulirish Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Usage - before

Usage - after

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardpeet commented Jan 31, 2018

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patrickhulce commented Jan 30, 2018 •

edited by paulirish

Loading