cli: add --print-config flag #6107

brendankenny · 2018-09-26T01:12:18Z

finally the last step fixing #1826 (more or less :)

after Config takes the input config JSON and extends it, merges passes, concats audit arrays, filters for --only-audits, merges options, etc, it can be difficult to visualize exactly what they end result will be. This flag makes that simple by printing the full, normalized config to stdout.

The only real issue with JSON.stringifying the config is the live implementation properties in audits and gatherers (as well as instance in gatherers), which can't be stringified. These are skipped.

However, if audits/gatherers were specified with a path (like all the core configs do), the output of --print-config is a valid LH.Config.Json and can be c/p into a new config file and used to create an identical LH run.

Currently not implemented possibilities:

if audits/gathers don't have a path, some kind of string replacement for the live implementation and instance properties to at least indicate that they were there (and throw an error if you pass it back into Config). Basically a more informative [object Object]. Live implementation seems mostly used for testing Lighthouse itself, though, so this may be necessary.
absolute paths. Could help debug why e.g. an audit isn't being loaded due to a bad relative path. This limits the copy/pastability of the output, though, and we have pretty good error messages in config.js to help in these cases already

I've also eliminated any empty audit/gatherer options objects, as they're noisy in the output, they're all empty, and they're optional in a LH.Config.Json anyways (they're immediately replaced with the same {} when normalized).

Happy to bikeshed on anything!

brendankenny · 2018-09-26T01:16:59Z

lighthouse-cli/bin.js

+    const config = generateConfig(cliFlags, configJson);
+    process.stdout.write(config.print());
+    return;
+  }


this needed to be in the full flow of the cli since it turns out process.stdout.write (and by extension console.log) isn't flushed or waited on by process.exit(), so the exit can come mid write and the JSON output will be truncated. Since index.js just requires this module (running the top-level code) and calls run(), this needed some way to prevent getting to run() without calling process.exit(), so...here we are.

(and might as well switch to async/await while in here)

brendankenny · 2018-09-26T01:18:02Z

lighthouse-cli/bin.js

+  if (typeof cliFlags.enableErrorReporting === 'undefined') {
+    cliFlags.enableErrorReporting = await askPermission();
+  }
+  if (cliFlags.enableErrorReporting) {


this check isn't strictly necessary (Sentry.init itself checks for enableErrorReporting), but added this just to reassure anyone reading this file and not digging in farther

brendankenny · 2018-09-26T01:19:04Z

lighthouse-core/lib/i18n/i18n.js

@@ -315,4 +319,5 @@ module.exports = {
  createMessageInstanceIdFn,
  getFormatted,
  replaceIcuMessageInstanceIds,
+  MESSAGE_INSTANCE_ID_REGEX,


for testing

add comment that it's exported only for testing?

add comment that it's exported only for testing?

I added an isIcuMessage() instead, just because we do the same check a couple of times internally, so nice in there and nicely encapsulated externally

paulirish

i like the idea.

paulirish · 2018-09-26T07:24:30Z

lighthouse-cli/cli-flags.js

-        if (!isListMode && !isOnlyAuditMode && argv._.length === 0) {
-          throw new Error('Please provide a url');
+        const isPrintConfigMode = argv.printConfig;
+        if (isListMode || isOnlyAuditMode || isPrintConfigMode) {


i think printConfigMode is actually captured under the spirit of the isListMode variable. should be renamed though. doPrintAndQuit ?

paulirish · 2018-09-26T07:29:11Z

lighthouse-core/index.js

+ *   not present, the default config is used.
+ * @return {Config}
+ */
+function generateConfig(flags, configJson) {


the order here is unexpected to me. i know lighthouse's signature matches this order, but ... can we switch to make it the same as the Config constructor

the order here is unexpected to me. i know lighthouse's signature matches this order, but ... can we switch to make it the same as the Config constructor

yeah, that's fine.

I think there's also a strong YAGNI argument for not making this a method at all and just have the CLI pull in Config, but it's nice not having the Config constructor in our public API, and in theory it's also nice to say "just give me a config file in the exact way core makes one"...it's just that that (currently?) just involves calling the constructor

paulirish · 2018-09-26T07:30:34Z

lighthouse-core/lib/i18n/i18n.js

@@ -315,4 +319,5 @@ module.exports = {
  createMessageInstanceIdFn,
  getFormatted,
  replaceIcuMessageInstanceIds,
+  MESSAGE_INSTANCE_ID_REGEX,


add comment that it's exported only for testing?

paulirish · 2018-09-26T07:30:56Z

lighthouse-core/lib/i18n/i18n.js

+ * Recursively walk the input object, looking for property values that are
+ * string references and replace them with their localized values. Primarily
+ * used with the full LHR as input.
+ * @param {*} lhr


why not {LH.Result} ?

why not {LH.Result} ?

I pass in the config now to get a translated one. I can rename the param to make that more obvious

patrickhulce

looks print-y good to me! ;)

patrickhulce · 2018-09-26T13:35:25Z

lighthouse-core/config/config.js

+   * Audit `implementation` and `instance` do not survive this process.
+   * @return {string}
+   */
+  print() {


🚲 🏠 : print makes me think it will print it not just return the string :)

formatAsString getPrintString stringify getDisplayString

any of those strike your fancy?

going with getPrintString unless anyone has other votes :) I don't care a whole lot, though it would be nice if whatever we choose communicates that the string result was functional in nature, not just pretty printing or something

patrickhulce · 2018-09-26T13:36:39Z

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

+      assert.ok(localizableCount > 0);
+    });
+
+    it('prints a valid ConfigJson that can make an identical Config', () => {


❤️ this test

patrickhulce · 2018-09-26T13:38:47Z

lighthouse-cli/test/cli/index-test.js

+      const ret = spawnSync('node', [indexPath, '--print-config'], {encoding: 'utf8'});
+
+      const config = JSON.parse(ret.stdout);
+      assert.strictEqual(config.settings.output[0], 'html');


I think this would actually be a good candidate for snapshot testing, but I know you're not a fan :)

good candidate for snapshot testing, but I know you're not a fan

I'd make my opinion more nuanced in that if there's constant snapshot churn they lose efficacy as a unit test because we stop paying attention, but I agree this is a good candidate (default config shouldn't change too much...) and less awkward than this style of checking.

We do lose the immediacy of the 'html' here vs the 'json' in the test below (and auditMode true vs false), but that might be ok(ish).

I'll add it.

What I'm not a fan of is expect(result).to.not.be.checked.in.a.ridiculous('manner') :P:P

haha alright, very fair. jest isn't big on the dot chains though! at least not nearly as bad as chai

cli: add --print-config flag

ef9757b

brendankenny requested review from patrickhulce and paulirish as code owners September 26, 2018 01:12

brendankenny commented Sep 26, 2018

View reviewed changes

readme

aab4134

paulirish reviewed Sep 26, 2018

View reviewed changes

patrickhulce reviewed Sep 26, 2018

View reviewed changes

brendankenny added 3 commits September 26, 2018 12:25

lotsofeedback

1c45b83

Merge branch 'master' into printconfig

5e1629e

more

b3af33a

paulirish approved these changes Sep 26, 2018

View reviewed changes

brendankenny merged commit 34b55b3 into master Sep 27, 2018

brendankenny deleted the printconfig branch September 27, 2018 00:49

brendankenny mentioned this pull request Jan 7, 2019

misc: clean up local names within Config #6950

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: add --print-config flag #6107

cli: add --print-config flag #6107

brendankenny commented Sep 26, 2018 •

edited

Loading

brendankenny Sep 26, 2018 •

edited

Loading

brendankenny Sep 26, 2018

brendankenny Sep 26, 2018

paulirish Sep 26, 2018

brendankenny Sep 26, 2018

paulirish left a comment

paulirish Sep 26, 2018

brendankenny Sep 26, 2018

paulirish Sep 26, 2018

brendankenny Sep 26, 2018 •

edited

Loading

paulirish Sep 26, 2018

paulirish Sep 26, 2018

brendankenny Sep 26, 2018

patrickhulce left a comment

patrickhulce Sep 26, 2018

brendankenny Sep 26, 2018

patrickhulce Sep 26, 2018

patrickhulce Sep 26, 2018

brendankenny Sep 26, 2018

brendankenny Sep 26, 2018

patrickhulce Sep 26, 2018

cli: add --print-config flag #6107

cli: add --print-config flag #6107

Conversation

brendankenny commented Sep 26, 2018 • edited Loading

brendankenny Sep 26, 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

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

brendankenny Sep 26, 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

patrickhulce 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

brendankenny commented Sep 26, 2018 •

edited

Loading

brendankenny Sep 26, 2018 •

edited

Loading

brendankenny Sep 26, 2018 •

edited

Loading