core: unify config and CLI settings #4849

patrickhulce · 2018-03-22T18:41:33Z

with this you can

filter audits/categories from CLI i.e. lighthouse --only-categories seo https://google.com 🎉 cc @rviscomi FYI
use all CLI flags in your config.settings object i.e. {settings: {disableDeviceEmulation: true}}

precursor to landing the throttling changes we discussed we need to eliminate this config.settings/flags discrepancy which is a big enough change on its own to split out to this separate PR (and a huge win on its own! 🎉 😃)

essentially this PR eliminates all references to .flags beyond the initial index.js that calls new Config to merge the flags into settings

also cleans up some naming discrepancy that config was used to refer to passConfig which became super confusing when we actually needed access to config.settings

options -> passContext when applicable
config -> passConfig when applicable
flags -> settings

patrickhulce · 2018-03-22T18:44:24Z

lighthouse-core/config/default.js

@@ -5,10 +5,14 @@
 */
 'use strict';

+const Driver = require('../gather/driver');


I'm not a fan of requiring core files in default config, but we'll need a strategy for this when it comes to throttling soon. split constants out to a separate file? have them canonically live in config?

if those settings like max_wait_for_fully_loaded will live in settings they can probably be removed from the original file so why not put them in here to start with? we could export them from this file so other files that might need it as a fallback can still get those values

well that's the question up for discussion :)

it makes some sense to continue having them colocated with the logic they affect as well as to simplify unit testing where a full default config is not given (think the lantern simulation settings for example)

I take it you vote to keep them in the config?

+1 for putting these constants in the config. Requiring the driver from cli-flags was the absolute worst. Basing them in config seems most natural.

@patrickhulce yeah +1 for config

patrickhulce · 2018-03-22T18:44:58Z

lighthouse-core/config/config.js

@@ -563,8 +565,13 @@ class Config {
   * @return {!Set<string>}
   */
  static getAuditIdsInCategories(categories) {
-    const audits = _flatten(Object.keys(categories).map(id => categories[id].audits));
-    return new Set(audits.map(audit => audit.id));
+    /** @type {Array<string>} */


I wanted to try to do some type checking in config, did 2 changes and then scared off by the sea of red. save that for later :)

when you say @brendankenny 3 times, your type checking will be resolved automatically

yeah, it's the worst file :) We might have to do it a few methods at a time, turning on type checking for changes and then turning it back on when committing so the build stays green :)

paulirish

looks pretty awesome!

I like how flags and settings come together in beautiful harmony and the rest of the pipeline gets to consider it as one.

paulirish · 2018-03-23T02:08:53Z

lighthouse-core/config/default.js

@@ -5,10 +5,14 @@
 */
 'use strict';

+const Driver = require('../gather/driver');


+1 for putting these constants in the config. Requiring the driver from cli-flags was the absolute worst. Basing them in config seems most natural.

paulirish · 2018-03-23T02:11:45Z

lighthouse-core/runner.js

@@ -21,7 +21,7 @@ const Sentry = require('./lib/sentry');
 class Runner {
  static run(connection, opts) {
    // Clean opts input.
-    opts.flags = opts.flags || {};


you can also remove flags from return Runner.run(connection, {url, flags, config}) in index.js

paulirish · 2018-03-23T02:13:12Z

typings/externs.d.ts

+      settings: ConfigSettings;
+    }
+
+    export interface ConfigSettings {


technically doesn't this extend Flags ?

yeah we'll need to decide what to do here, flags is currently what comes back fresh off yargs, which is slightly different than the flags that get passed into core which is what ConfigSettings extends, I'll extract the shared stuff and have both extend that 👍

brendankenny · 2018-03-23T20:38:42Z

lighthouse-core/gather/driver.js

@@ -28,12 +28,10 @@ const DEFAULT_PAUSE_AFTER_LOAD = 0;
 const DEFAULT_NETWORK_QUIET_THRESHOLD = 5000;
 // Controls how long to wait between longtasks before determining the CPU is idle, off by default
 const DEFAULT_CPU_QUIET_THRESHOLD = 0;
+// Controls maximum amount of time to wait before continuing
+const DEFAULT_MAX_WAIT_FOR_LOAD = require('../config/constants').MAX_WAIT_FOR_LOAD;


we might not be able to do this now (lots of test modifications?), but it would be pretty cool to pull all the references to these constants out of driver altogether, and just have the various pauseAfterLoadMs, etc (basically all of ConfigSettings) required on the config by the time it gets into driver

yeah sg for followup 👍

to clarify, this wouldn't be only about a neat architecture or whatever. It would also let you --print-config or whatever so you could get a full print out of exactly what the LH run will be like (and what things you could provide instead) without having to look up defaults or whatever

brendankenny · 2018-03-23T20:45:55Z

typings/externs.d.ts


 declare global {
-  module LH {
+  namespace LH {


was this change necessary? I don't actually remember the reasoning (and it's possible the reasons no longer apply as this file changed), but I do remember it was a deliberate decision at the time :)

no it keeps getting automatically changed or a bad rebase or something :)

brendankenny · 2018-03-23T20:50:46Z

lighthouse-core/config/constants.js

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+'use strict';


might be dumb, but: maybe defaults.js or something? It might be good to have it be specifically a place where we keep defaults for these kinds of controls and avoid the temptation of just having it be a grab bag of constants we need across files

point taken but can we please pick another name? default.js and defaults.js would just be nuts 😆 maybe we rename default.js -> default-config.js?

haha, forgot about default.js.

default.js -> default-config.js actually makes a lot of sense, but not sure if you want to do that in this PR. Maybe we don't reference it that many places so it's not too bad?

yeah agreed, let's do this in followup, how about I just leave the Driver require for now (revert constants.js) and address all these in separate go for review?

patrickhulce · 2018-03-23T23:00:50Z

so this look good to you folks then? 😃 @paulirish @brendankenny

paulirish

lgtm from me

paulirish · 2018-03-26T21:56:31Z

lighthouse-core/gather/driver.js

+      let pauseAfterLoadMs = passConfig && passConfig.pauseAfterLoadMs;
+      let networkQuietThresholdMs = passConfig && passConfig.networkQuietThresholdMs;
+      let cpuQuietThresholdMs = passConfig && passConfig.cpuQuietThresholdMs;
+      let maxWaitMs = passContext.settings && passContext.settings.maxWaitForLoad;


wanna clean this up?

could do..

const passConfig = passContext.passConfig || {}; // dunno if the `|| {} ` is required const {pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs, maxWaitMs} = passConfig;

paulirish · 2018-03-26T22:01:17Z

typings/externs.d.ts

-      disableCpuThrottling?: boolean;
-      disableNetworkThrottling?: boolean;
+
+      extraHeaders?: string;


this one doesnt belong in sharedflagsettings?

correct, in LHFlags it's a string but in ConfigSettings its an object of headers

patrickhulce · 2018-03-26T23:49:38Z

@brendankenny does this have your ✝️🙏?

brendankenny

two last comments/suggestions, but LGTM!

brendankenny · 2018-03-23T21:40:51Z

lighthouse-core/gather/gather-runner.js

   * @return {!Promise}
   */
-  static loadPage(driver, options) {
-    return driver.gotoURL(options.url, {
+  static loadPage(driver, passContext) {


it's a little weird that the passContext received here isn't (quite) the same object/shape as the options object passed on to gotoURL (now also called passContext on the receiver side). Can we unify?

I guess if I were being real about it, I'd leave gotoURL as options that is {waitForLoad: boolean, passContext: Object} rather than abuse passContext, but the layer of nesting was annoying. I'm guessing you're in favor of doing that instead? :)

ha, it is more awkward but probably better. It'll get better when we pull out a passContext type :)

brendankenny · 2018-03-23T21:42:45Z

lighthouse-core/config/config.js

   */
-  constructor(configJSON, configPath) {
+  constructor(configJSON, configPath, flags) {


should we drop the optional configPath since it's on flags now?

good call done 👍

core: unify config and CLI settings

c15fdb4

patrickhulce requested review from brendankenny and paulirish as code owners March 22, 2018 18:41

patrickhulce commented Mar 22, 2018

View reviewed changes

devtools-bot added the waiting4reviewer label Mar 22, 2018

patrickhulce added 2 commits March 22, 2018 15:51

fix driver test timeout

cad27c4

fix tsc error

e5c40cd

paulirish reviewed Mar 23, 2018

View reviewed changes

patrickhulce added 2 commits March 23, 2018 08:43

feedback

8258fbc

tests

889091a

brendankenny reviewed Mar 23, 2018

View reviewed changes

patrickhulce added 2 commits March 23, 2018 15:53

revert constants

13aee2b

revert module -> namespace

43e04d0

patrickhulce added 3 commits March 23, 2018 16:23

fix driver test

80750bc

fix revert bugs

644e5f4

cli test

c5b3dec

This was referenced Mar 26, 2018

Replace throttling flags with throttlingMethod #4871

Closed

Convert metrics to read score thresholds from options if set #4873

Closed

paulirish approved these changes Mar 26, 2018

View reviewed changes

pauls feedback

fbc6a5e

brendankenny approved these changes Mar 27, 2018

View reviewed changes

brendan feedback

95fa8c5

patrickhulce merged commit ecb3a8c into master Mar 27, 2018

patrickhulce deleted the unify_config_cli_settings branch March 27, 2018 22:15

paulirish removed the waiting4reviewer label Apr 30, 2018

core: unify config and CLI settings #4849

core: unify config and CLI settings #4849

Conversation

patrickhulce commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

wardpeet Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

patrickhulce Mar 22, 2018 • 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

paulirish left a 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

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

patrickhulce Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Mar 23, 2018 • edited Loading

paulirish left a 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

patrickhulce commented Mar 26, 2018

brendankenny left a 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

patrickhulce commented Mar 22, 2018 •

edited

Loading

wardpeet Mar 22, 2018 •

edited

Loading

patrickhulce Mar 22, 2018 •

edited

Loading

patrickhulce Mar 23, 2018 •

edited

Loading

patrickhulce commented Mar 23, 2018 •

edited

Loading