Support dynamic plugins via plugin objects stored in lighthouse config

[mdittmer]

It's more flexible for Lighthouse to support plugin classes provided from somewhere other than the filesystem.

Example use case: Library foo wants to consume and extend Lighthouse via a [[foo-instance]] => [[lighthouse-plugin-type]] adapter. That means that the plugin classes are not static modules, but adapted from foo's structures.

E.g.,

// run-foo-in-lighthouse.js
// ...
lighthouse({
  // ...
  passes: {
    // ...
    gatherers: ['some-core-gatherer', fooObj.toLighthouseGatherer()]
  },
  // ...
  audits: ['some-core-audit', anotherFooObj.toLighthouseAudit()]
});

[mdittmer]

@brendankenny PTAL!

[brendankenny]

Hi Mark! Thanks for taking the time to get into this.

This is definitely needed. No reason for someone using Lighthouse as a module to have to write their audits to disk in order to use them :)

[brendankenny]

I don't know if we necessarily want to require the type hierarchy (e.g. someone is using a class for multiple reasons and it also happens to work as a LH audit), so we do have the runtime structural(ish) typing in assertValidAudit below.

What if instead here it did something like

let AuditClass;
if (typeof audit === 'string') {
  // See if the audit is a Lighthouse core audit.
  const coreAudit = coreList.find(a => a === `${audit}.js`);
  // existing code, etc
  AuditClass = require(requirePath);
} else {
  AuditClass = audit;
  audit = audit.meta.name;
}

// Confirm that the audit appears valid.
assertValidAudit(audit, AuditClass);

return AuditClass;

the only real issues with this solution are

• audit is becoming an increasingly poor descriptive variable name here :) Feel free to switch to something better.
• audit.meta.name might not exist and assertValidAudit does a better job of giving feedback if it doesn't. Maybe we should switch to something like assertValidAudit(AuditClass, auditWhatever);, have auditWhatever be optional, and let assertValidAudit handle the fallback to meta.name and even AuditClass.name if need be?

[mdittmer]

Done.

[brendankenny]

this makes me sad, but it's fine for this PR. It's our own fault for using JSON parsing for deep copying. We probably need a real deep copy thats aware of the config format to allow for exceptions like this.

[mdittmer]

I agree. You rightly identified that (for this PR) I just implemented the shortest path to making this work. I figure someone more familiar with Lighthouse than me might want design input on some kind of conditional-cloner.

[brendankenny]

Unless I'm missing something tricky we've done here, I believe you're going to have to do a nested copy here. passes is an array of objects, each with its own array of gatherers

[mdittmer]

You're right. I'll fix it.

[brendankenny]

I think it would be preferable to leave resolvePlugin as strictly string input and output, roughly analogous to require.resolve.

If you take the code you're adding to requireAudits in config.js and also add it to getGathererClass in gather-runner.js, the live plugin class will never make it this far, as those two spots can handle that case, and this can stay strings-only

[mdittmer]

Done.

[brendankenny]

may also want to add a test to getGathererClass on GatherRunner that it correctly exposes and validates a live gatherer

[mdittmer]

Done.

[mdittmer]

Applied requested changes. I made the "name" parameter to assertValid*() functions optional, as suggested.

[brendankenny]

LGTM. Thanks!