cli: add --plugins flag to load from the command line #7407

brendankenny · 2019-03-07T00:42:22Z

Should make e.g. the recipe a lot clearer since you don't need a config file.

Old:

{
  extends: 'lighthouse:default',
  plugins: ['lighthouse-plugin-preload-as'],
}

lighthouse --config-path=my-config.json ...

New:
lighthouse --plugins=lighthouse-plugin-preload-as ...

Two things:

--plugins or --plugin? --plugin seems like it might be more natural (--plugin=lighthouse-plugin-preload-as) and it's what e.g. eslint does, but technically you can have multiple values (--plugin=plugin1 plugin2), plugins is the property name in the config, and we use the plural for other possibly single things, e.g. --only-categories.
Since plugins isn't a setting (it's a top level field in the config), it can't go through our normal flags/settings pipeline and instead has to be yet another optional argument to run.runLighthouse() and the lighthouse() module interface. This is ugly enough that I almost don't want to do it, but we've already talked about switching to an options object for v5, so maybe it's fine for now? (see below)

brendankenny · 2019-03-07T00:43:56Z

lighthouse-cli/test/cli/run-test.js

 const getFlags = require('../../cli-flags').getFlags;

 describe('CLI run', function() {
-  it('runLighthouse completes a LH round trip', () => {
-    const url = 'chrome://version';
+  describe('LH round trip', () => {


split this up as it seems like there are a few different things being tested (and channel is about to be another check added to this)

brendankenny · 2019-03-07T00:44:16Z

lighthouse-cli/test/cli/run-test.js

+
+    it('returns results that match the saved results', () => {
+      const {lhr} = passedResults;
+      assert.equal(fileResults.audits.viewport.rawValue, false);

      // passed results match saved results


no idea why this is being checked this way, but going with it :)

brendankenny · 2019-03-07T00:45:21Z

lighthouse-core/test/config/config-test.js

-      assert.strictEqual(groupIds[groupIds.length - 1], 'lighthouse-plugin-simple-new-group');
+    });
+
+    it('should append and use plugin-prefixed groups', () => {


@jburger424 I split the should append audits and a group in two because I was having trouble following it all in one :) Should be exactly the same functionality

👍 this is cleaner

patrickhulce

Since plugins isn't a setting (it's a top level field in the config), it can't go through our normal flags/settings pipeline and instead has to be yet another optional argument to run.runLighthouse() and the lighthouse() module interface.

I think we should avoid adding plumbing through lighthouse index/runLighthouse. AFAICT, plugins are purely a config concern that don't have any other impacts to core, so it's certainly possible to contain the impact to CLI and config.

What if instead, CLI could call into Config.mergePlugins if the plugins CLI flag was set? It's already doing the work to figure out configJSON based on a set of flags that affect the config, so adding one more conditional to check for plugins seems like a small price to pay for nuking all the other complexity to our entrypoints that would be introduced otherwise.

WDYT?

brendankenny · 2019-03-07T17:44:19Z

What if instead, CLI could call into Config.mergePlugins if the plugins CLI flag was set? It's already doing the work to figure out configJSON based on a set of flags that affect the config, so adding one more conditional to check for plugins seems like a small price to pay for nuking all the other complexity to our entrypoints that would be introduced otherwise.

We could do this now (generateConfig is already exposed for --print-config, and might be a nice spot for this?), though the other entry points are going to have to do something like this too (joining categoryIds as an option passed in) to support plugins, so is it better to just centralize now?

brendankenny · 2019-03-07T18:06:07Z

I think we'll want node module users to be able to use this too, but that just made me realize I made this way too complicated (I think).

We don't want it in settings, but it can still be on flags, which is passed into the config constructor. I think this can be dealt with entirely within config.js.

patrickhulce · 2019-03-07T19:02:51Z

We don't want it in settings, but it can still be on flags, which is passed into the config constructor. I think this can be dealt with entirely within config.js.

Perfect 👍

brendankenny · 2019-03-07T20:03:52Z

ah, so much better

brendankenny · 2019-03-07T20:05:37Z

lighthouse-cli/test/cli/run-test.js


-      fs.unlinkSync(filename);
+    it('merged the plugin into the config', () => {


this is well beyond just stretching the "unit" part of "unit tests", but this check has trivial overhead and until we have a smoketest for plugins I'd like to leave this in

patrickhulce · 2019-03-07T20:43:58Z

lighthouse-cli/test/cli/run-test.js

@@ -19,42 +19,73 @@ const fastConfig = {
  },
 };

+// Map plugin name to fixture since not actually installed in node_modules/.
+jest.mock('lighthouse-plugin-simple', () => {
+  return require('../../../lighthouse-core/test/fixtures/config-plugins/lighthouse-plugin-simple');


.js of the real file?

or we are making it work like a package so it's ok?

or we are making it work like a package so it's ok?

I was making it like a package (I added a / to make it clear it's a directory), but can also make it the js file if you'd rather

nah, it's cool

patrickhulce · 2019-03-07T20:52:50Z

lighthouse-core/test/config/config-test.js

+        },
+      };
+      const baseFlags = {configPath: configFixturePath};
+      const simplePlugin = 'lighthouse-plugin-simple';


nit: could we go with PluginName here? just easier to remember down below it's a string

patrickhulce · 2019-03-07T20:54:50Z

lighthouse-core/config/config.js

   * @param {string=} configDir
   * @return {LH.Config.Json}
   */
-  static mergePlugins(configJSON, configDir) {
-    const pluginNames = configJSON.plugins;
+  static mergePlugins(configJSON, flags, configDir) {


these are like the only meaningful changes, right?

so clean ✨ 😃

these are like the only meaningful changes, right?

yep, that's it! soooo much better

haha, I also just realized this was the original plan all along, so at least we got back to it :)

so it seems ok to add this to the handful of special-cased flag/config mergings in config.js when we add support for the CLI flag.

#6959 (comment)

brendankenny requested review from patrickhulce and paulirish as code owners March 7, 2019 00:42

brendankenny commented Mar 7, 2019

View reviewed changes

patrickhulce suggested changes Mar 7, 2019

View reviewed changes

brendankenny added 3 commits March 7, 2019 11:32

cli: add flag to load plugins from command line

9e3da7f

add tests

374b923

test name

369b310

brendankenny force-pushed the cliplugins branch from 5b0399f to 369b310 Compare March 7, 2019 19:34

use flags, not cliFlags

62d08c4

brendankenny commented Mar 7, 2019

View reviewed changes

patrickhulce reviewed Mar 7, 2019

View reviewed changes

feedback

2e422e8

patrickhulce approved these changes Mar 7, 2019

View reviewed changes

brendankenny merged commit a3b9cf3 into master Mar 7, 2019

brendankenny deleted the cliplugins branch March 7, 2019 22:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: add --plugins flag to load from the command line #7407

cli: add --plugins flag to load from the command line #7407

brendankenny commented Mar 7, 2019 •

edited

Loading

brendankenny Mar 7, 2019

brendankenny Mar 7, 2019

brendankenny Mar 7, 2019

jburger424 Mar 7, 2019

patrickhulce left a comment

brendankenny commented Mar 7, 2019

brendankenny commented Mar 7, 2019

patrickhulce commented Mar 7, 2019

brendankenny commented Mar 7, 2019

brendankenny Mar 7, 2019

patrickhulce Mar 7, 2019 •

edited

Loading

brendankenny Mar 7, 2019

patrickhulce Mar 7, 2019

patrickhulce Mar 7, 2019

brendankenny Mar 7, 2019

patrickhulce Mar 7, 2019

brendankenny Mar 7, 2019

brendankenny Mar 7, 2019


		fs.unlinkSync(filename);
		it('merged the plugin into the config', () => {

cli: add --plugins flag to load from the command line #7407

cli: add --plugins flag to load from the command line #7407

Conversation

brendankenny commented Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny commented Mar 7, 2019

brendankenny commented Mar 7, 2019

patrickhulce commented Mar 7, 2019

brendankenny commented Mar 7, 2019

Choose a reason for hiding this comment

patrickhulce Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Mar 7, 2019 •

edited

Loading

patrickhulce Mar 7, 2019 •

edited

Loading