core(config): augment settings/passes with defaults #4894

patrickhulce · 2018-03-30T00:21:37Z

~~WIP, but I'm waiting to fix all the tests and do cleanup until we get some consensus on what this looks like :)~~ updated all the tests!

This PR would move all defaults that can be specified in the config to a config/constants.js file, enforces that all configs implicitly extend default, and changes the defaults specified across various files to be pulled in from constants. This PR also does some promised hygiene on our config files.

Points of discussion with my opinions:

Do we like that all configs extend default?
Yes we can now make more assumptions about the config and most valid use cases for truly custom can be achieved through onlyAudits/onlyCategories/skipAudits/etc
Do we like all constants/defaults live in a separate file?
Yes it will be super convenient for scanning all tunable parameters and it's easy to require the constants file in to all the places we still want to double-default parameters for more convenient testing, etc.
????

cc @benschwarz in case you're doing any custom config magic we might want to know about :)

paulirish

lgtm % nits

paulirish · 2018-03-30T21:44:07Z

lighthouse-core/config/constants.js

+
+module.exports = {
+  throttling,
+  defaultSettings: {


might as well extract this obv to a var like the throttling one.
and same with defaultPassConfig.

paulirish · 2018-03-30T21:44:43Z

lighthouse-cli/bin.js

@@ -55,8 +54,6 @@ if (cliFlags.configPath) {
  // Resolve the config file path relative to where cli was called.
  cliFlags.configPath = path.resolve(process.cwd(), cliFlags.configPath);
  config = /** @type {!LH.Config} */ (require(cliFlags.configPath));
-} else if (cliFlags.perf) {


you're removing --perf ? omg

can we add an alias for --onlyCategories=perf (is that it?)

yeah added back the flag as an alias for --only-categories=performance for now, but IMO we should get rid of it for v3 :)

patrickhulce · 2018-03-30T22:23:31Z

@brendankenny this overall direction look good enough to you for me to change all tests?

brendankenny

Added some general comments on the current version. Overall, though, I really like just using all the settings from the default config.

Copying over the the default passes and audits seems a lot more complicated, though, at least with our current overriding semantics. It seems easy to express "add these additional things", but much harder to say "I want to run only these new things", unless I'm missing how it works :)

As a result, the onlyCategories: ['mixedContent'], in mixed-content-config.js ends up being a pretty subtle line for such non-trivial effects (again, if I'm reading this correctly :)

brendankenny · 2018-04-02T18:30:15Z

lighthouse-core/config/default-config.js

-      cpuSlowdownMultiplier: emulation.CPU_THROTTLE_METRICS.rate,
-    },
-  },
+  settings: constants.defaultSettings,


maybe this is still better broken out as an object where the properties are set to individual constants? It would be nice if the shape of default-config.js was still readable in place, even if the actual numeric values require a lookup

I'd be against that since it feels like a small when at the cost of manual/error-prone work to copy properties from one file to the other and muddies the source of truth, the shape of settings is a cmd+click away still :)

Maybe it's just me, @paulirish WDYT?

at first i was like "yah" but now i'm like "nah"

having the keynames present would be nice, but mostly a tease if the actual values aren't there. might as well be clear that the source of truth is not this file.

so i'm +1 on keeping it as is in the PR

brendankenny · 2018-04-02T18:35:26Z

docs/readme.md


 ```js
-const perfConfig: any = require('lighthouse/lighthouse-core/config/perf.json');
+const perfConfig = {settings: {onlyCategories: ['performance']}};


this is a nice example though is it possible do we want to get rid of perf-config.js? There are more potential tweaks that could be made there beyond just onlyCategories

yeah just brought it back since you and paul are both against

brendankenny · 2018-04-02T18:36:24Z

docs/readme.md

 // ...
 launchChromeAndRunLighthouse(url, flags, perfConfig).then( // ...
 ```

-You can also craft your own config (e.g. [plots-config.js](https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/config/plots-config.js)) for completely custom runs. Also see the [basic custom audit recipe](https://github.com/GoogleChrome/lighthouse/tree/master/docs/recipes/custom-audit).
+You can also craft your own config (e.g. [mixed-content-config.js](https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/config/mixed-content-config.js)) for custom runs. Also see the [basic custom audit recipe](https://github.com/GoogleChrome/lighthouse/tree/master/docs/recipes/custom-audit).


recipes/custom-audit

does this still work? we should probably have a test for that...

heh, yeah I updated the recipe to nuke the extends property

brendankenny · 2018-04-02T18:39:16Z

lighthouse-cli/bin.js

@@ -56,7 +55,7 @@ if (cliFlags.configPath) {
  cliFlags.configPath = path.resolve(process.cwd(), cliFlags.configPath);
  config = /** @type {!LH.Config} */ (require(cliFlags.configPath));
 } else if (cliFlags.perf) {
-  config = /** @type {!LH.Config} */ (perfOnlyConfig);
+  cliFlags.onlyCategories = ['performance'];


does this get weird if they supply a config and specify --perf? The old overwriting config completely seems more predictable

yeah just brought it back since you and paul are both against

brendankenny · 2018-04-02T18:42:22Z

lighthouse-cli/test/smokehouse/perf/run-tests.sh

@@ -3,8 +3,8 @@
 node lighthouse-cli/test/fixtures/static-server.js &

 sleep 0.5s
-
-config="lighthouse-core/config/perf.json"
+# TODO: replace this with a smoketest for --only-categories performance and nuke the perf-config


pull out into a tracking issue (or add to breaking changes umbrella)? Bigger change than just dropping the smoke test :)

nuked since we'll keep the config around

brendankenny · 2018-04-02T18:43:32Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

@@ -13,7 +13,7 @@

 const Audit = require('./audit');
 const URL = require('../lib/url-shim');
-const Emulation = require('../lib/emulation');
+const mobile3G = require('../config/constants').throttling.mobile3G;


nit: to keep the more descriptive name, maybe const targetLatencyMs = require('../config/constants').throttling.mobile3G.rttMs?

brendankenny · 2018-04-02T18:47:55Z

lighthouse-core/config/config.js


+const isEqual = require('lodash.isequal');


this is no problem for longtime lodash users, I'm sure, but any chance of calling the local deepEqual? :)

patrickhulce · 2018-04-02T20:29:44Z

but much harder to say "I want to run only these new things", unless I'm missing how it works :)
As a result, the onlyCategories: ['mixedContent'], in mixed-content-config.js ends up being a pretty subtle line for such non-trivial effects (again, if I'm reading this correctly :)

I'm not sure if it's harder to say I want to run only these things since it's still just 1 prop you stick in the config, but I agree the "everything extends default" makes things more confusing as to what will be run. It'd be great to know how anyone not on the Lighthouse team feels about these changes and if the ergonomic win is worth it, @benschwarz @denar90 any reactions to always extend default config?

My sneaking suspicion is that onlyCategories: ['mixedContent'] is subtle to those who know too much about how config.js works, from the user's perspective if onlyCategories/onlyAudits always just gives you those categories/audits in the most efficient way then what's the problem? :)

patrickhulce · 2018-04-02T21:55:15Z

ok updated all the tests, it actually wasn't that bad. 2 major consequences:

we now support filtering of the config in conjunction with plugin injection (the only way to run just the a few custom programmatic audits)
we no longer throw on a few previously invalid cases because we'll default the passes/config so that it looks good

brendankenny · 2018-04-03T18:49:14Z

A solution in the middle could be to pull over settings from the default config and not passes/audits/categories (or, equivalently, have a base audit with empty passes/audits/categories). I'm not sure how that would work with individual pass settings, though (back to default constants instead of from a base config?)

Some of the tests aren't unreasonable cases, like "only run these gatherers, not audits or categories" (and other gatherers?), and having to use onlyCategories or -G to not run the audits and categories added by default seems like a step back.

This does seem to be trading mental complexity for a nice model of config defaults...

denar90 · 2018-04-03T19:53:11Z

lighthouse-core/config/plots-config.js

@@ -6,20 +6,13 @@
 'use strict';

 module.exports = {


Looks like we don't need our own config anymore 💃
https://github.com/paulirish/pwmetrics/blob/v3.2.0/lib/lh-config.ts

brendankenny

Nice! Really like the simplifications in config.js

brendankenny · 2018-04-04T19:22:39Z

lighthouse-cli/bin.js

@@ -15,8 +15,9 @@ const runLighthouse = require('./run').runLighthouse;

 const log = require('lighthouse-logger');
 // @ts-ignore
-const perfOnlyConfig = require('../lighthouse-core/config/perf.json');
-const mixedContentConfig = require('../lighthouse-core/config/mixed-content.js');
+const perfOnlyConfig = require('../lighthouse-core/config/perf-config.js');


does this still need to be @ts-ignore? I think that was because it always has an error on trying to require a json file, so it shouldn't be an issue with a js file. OTOH it might be any or hopelessly typed :)

nope, totally unnecessary now good call 👍

patrickhulce requested review from brendankenny, paulirish and vinamratasingal-zz as code owners March 30, 2018 00:21

devtools-bot added the waiting4reviewer label Mar 30, 2018

paulirish approved these changes Mar 30, 2018

View reviewed changes

patrickhulce mentioned this pull request Apr 2, 2018

core(lantern): cleanup Simulator construction #4910

Merged

brendankenny requested changes Apr 2, 2018

View reviewed changes

patrickhulce changed the title ~~[WIP] core(config): all configs must extend default~~ core(config): all configs must extend default Apr 2, 2018

denar90 reviewed Apr 3, 2018

View reviewed changes

core(config): augment settings/passes with defaults

aca2ba0

patrickhulce force-pushed the config_defaults branch from 0103ff8 to aca2ba0 Compare April 3, 2018 21:01

patrickhulce changed the title ~~core(config): all configs must extend default~~ core(config): augment settings/passes with defaults Apr 3, 2018

brendankenny approved these changes Apr 4, 2018

View reviewed changes

unnecessary tsignore

96ce7ff

patrickhulce merged commit be055b1 into master Apr 4, 2018

patrickhulce deleted the config_defaults branch April 4, 2018 22:03

paulirish removed the waiting4reviewer label Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(config): augment settings/passes with defaults #4894

core(config): augment settings/passes with defaults #4894

patrickhulce commented Mar 30, 2018 •

edited

Loading

paulirish left a comment

paulirish Mar 30, 2018

patrickhulce Mar 30, 2018

paulirish Mar 30, 2018

paulirish Mar 30, 2018

patrickhulce Mar 30, 2018 •

edited

Loading

patrickhulce commented Mar 30, 2018

brendankenny left a comment

brendankenny Apr 2, 2018

patrickhulce Apr 2, 2018

paulirish Apr 3, 2018

brendankenny Apr 2, 2018

patrickhulce Apr 2, 2018

brendankenny Apr 2, 2018

patrickhulce Apr 2, 2018

brendankenny Apr 2, 2018

patrickhulce Apr 2, 2018

brendankenny Apr 2, 2018

patrickhulce Apr 2, 2018

brendankenny Apr 2, 2018

patrickhulce Apr 2, 2018

brendankenny Apr 2, 2018

patrickhulce Apr 2, 2018

patrickhulce commented Apr 2, 2018

patrickhulce commented Apr 2, 2018

brendankenny commented Apr 3, 2018 •

edited

Loading

denar90 Apr 3, 2018

brendankenny left a comment

brendankenny Apr 4, 2018

patrickhulce Apr 4, 2018

core(config): augment settings/passes with defaults #4894

core(config): augment settings/passes with defaults #4894

Conversation

patrickhulce commented Mar 30, 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 Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

patrickhulce commented Mar 30, 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

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 commented Apr 2, 2018

patrickhulce commented Apr 2, 2018

brendankenny commented Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

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

patrickhulce commented Mar 30, 2018 •

edited

Loading

patrickhulce Mar 30, 2018 •

edited

Loading

brendankenny commented Apr 3, 2018 •

edited

Loading