cli: accept csv for array values #8933

connorjclark · 2019-05-11T19:25:21Z

node lighthouse-cli https://www.example.com/ --only-categories=performance,seo --only-categories=pwa

lighthouse-cli/cli-flags.js

patrickhulce

nice! no better motivation than a dev scorned 😆

lighthouse-cli/cli-flags.js

patrickhulce · 2019-05-13T15:53:47Z

lighthouse-cli/cli-flags.js

@@ -152,6 +161,17 @@ function getFlags(manualArgv) {
      .default('enable-error-reporting', undefined) // Undefined so prompted by default
      .default('channel', 'cli')
      .check(/** @param {LH.CliFlags} argv */ (argv) => {
+        // ".middleware" does not exist in this version of yargs, so do some preprocessing here.
+        for (const _key of Object.keys(argv)) {


I don't love that this is here and I'm not sure we can actually do this across the board.

(for example --chrome-flags="--window-size 800,600")

WDYT about just doing this to our specific .array properties minus blockedUrlPatterns?

ah ok. sounds like either way we need a hard coded list (either a blacklist of a whitelist). Whitelist is easier to reason about so let's do that :)

patrickhulce · 2019-05-13T15:55:46Z

lighthouse-cli/test/cli/cli-flags-test.js

@@ -28,4 +28,14 @@ describe('CLI bin', function() {
      assert.ok(optionsWithDescriptions.includes(opt), `cli option '${opt}' has no description`);
    });
  });
+
+  it('array values support csv', () => {
+    const flags = getFlags(


once we figure out other feedback would love the --window-size and blockedUrlPattern counter examples here too :)

blockedUrlPattern just b/c comma could be in a pattern? what if we could escape it?

nvm lets not do that :)

This reverts commit 9fe961a.

patrickhulce

lint is unhappy about the tests, but other than .check I think this LGTM! :)

patrickhulce · 2019-05-13T19:26:41Z

lighthouse-cli/cli-flags.js

@@ -152,6 +161,22 @@ function getFlags(manualArgv) {
      .default('enable-error-reporting', undefined) // Undefined so prompted by default
      .default('channel', 'cli')
      .check(/** @param {LH.CliFlags} argv */ (argv) => {
+        // ".middleware" does not exist in this version of yargs, so do some preprocessing here.


still not 100% sure that I love mutating the arguments in a .check method

WDYT of just saving the .argv to a variable and mutating that before we return in getFlags?

I think that might help make it more explicit that we are doing some non-standard yargs mutations

patrickhulce

LGTM!

Not sure if we want to add an example to show off our new muscles :)

patrickhulce · 2019-05-13T22:01:25Z

lighthouse-cli/cli-flags.js

@@ -171,6 +181,24 @@ function getFlags(manualArgv) {
          'For more information on Lighthouse, see https://developers.google.com/web/tools/lighthouse/.')
      .wrap(yargs.terminalWidth())
      .argv;
+
+  // ".middleware" does not exist in this version of yargs, so do some post-processing here.
+  /** @type {(keyof LH.CliFlags)[]} */


I think we're settling on Array<keyof LH.CliFlags> for complex types that aren't just string[] or w/e, though if you ask @brendankenny we're always settling on Array<*> ;)

though if you ask @brendankenny we're always settling on Array<*> ;)

well, yes, because it's an array, not a string, so why would you put that information last? :P They could have gone [string], but they used that up on tuples

brendankenny

typing is hard for cli-flags.js...

brendankenny · 2019-05-15T20:20:56Z

lighthouse-cli/cli-flags.js

-  return y.help('help')
+function getFlags(...manualArgv) {
+  const y = manualArgv.length ? yargs(manualArgv) : yargs;
+  /** @type {LH.CliFlags} */


this doesn't buy us much except having to coerce with /** @type {string[]} */ below

would like to keep /** @type {string[]} */ otherwise more errors come up. I added a comment saying why we can assume it to be a string[].

LH.CliFlags one could be removed if you want.

would like to keep /** @type {string[]} */ otherwise more errors come up.

but it's wrong because all of this is wrong :)

The ts-ignore was suppressing a different error than it said (I assume/hope it changed at some point), but the comment was still correct ('yargs() is incorrectly typed as not returning itself'): all the calls on yargs after help() are of type any and return any. In addition:

the type of LH.CliFlags[keyof LH.CliFlags] is definitely not a string array, really it should be LH.CliFlags['onlyAudits'|'onlyCategories'|'output'|'plugins'|'skipAudits']

if we pretend the type is string[] because yargs is returning that for the subset of keys we care about, assigning back as string[] is still not correct even for that subset because e.g. output is LH.OutputMode[], not string[] (and that's ignoring trying to reassign back to LH.CliFlags[keyof LH.CliFlags]). This will be an error in tsc 3.5+.

I think the most honest thing is to just leave as any internal to getFlags with a note about why and leave it up to tests to verify that getFlags really is returning LH.CliFlags.

lighthouse-cli/cli-flags.js

brendankenny · 2019-05-15T20:45:08Z

lighthouse-cli/cli-flags.js

-  const y = manualArgv ? yargs(manualArgv) : yargs;
-  return y.help('help')
+function getFlags(...manualArgv) {
+  const y = manualArgv.length ? yargs(manualArgv) : yargs;


this ends up somewhat confusing from the caller's perspective since it's no longer explicitly optional (even though we usually call it with no args) and when we do pass an argument it's still a single string (it's annoying that the yargs types require this because yargs definitely supports parsing just a string).

What about keeping it string= and doing const y = manualArgv ? yargs([manualArgv]) : yargs; instead?

I'm confused on how it's confusing to the caller?

if reverting the change, would you want the code using multiple args (admittedly it's just test code) to look like this?

getFlags([ 'http://www.example.com', '--only-categories=performance,seo', '--skipAudits=unused-javascript,redirects', '--skipAudits=bootup-time', ].join(' '));

or this?

getFlags('http://www.example.com --only-categories=performance,seo --skipAudits=unused-javascript,redirects --skipAudits=bootup-time');

if reverting the change, would you want the code using multiple args (admittedly it's just test code) to look like this?

yes from me. Maybe it's just that I haven't been completely indoctrinated to rest parameters yet, but to me an optional manualArgv?: string parameter is a lot clearer than ...manaulArgv: string[] at informing that the argument is optional. It's not a huge deal since it's not a commonly used interface, but it's only test code that uses this, so it makes sense to me to make the real use case clear in the signature.

brendankenny · 2019-05-15T20:46:21Z

lighthouse-cli/cli-flags.js

      .version(() => pkg.version)
      .showHelpOnFail(false, 'Specify --help for available options')

      .usage('lighthouse <url> <options>')
      .example(
          'lighthouse <url> --view', 'Opens the HTML report in a browser after the run completes')
+      .example(
+          'lighthouse <url> --only-categories=performance,pwa',


kick somewhere below? I'd call --only-categories less important than --config-path, --chrome-flags, etc

lighthouse-cli/cli-flags.js

brendankenny · 2019-05-15T20:59:43Z

lighthouse-cli/cli-flags.js

-  const y = manualArgv ? yargs(manualArgv) : yargs;
-  return y.help('help')
+function getFlags(...manualArgv) {
+  const y = manualArgv.length ? yargs(manualArgv) : yargs;


ha, actually, that test failure may be because our version of yargs doesn't take an array at all?

It passes locally.

(EDIT: misread the build log. one sec)

ok the issue is that if you pass in an array, you cannot have each item have multiple args. test just needs to change

also I guess they expect some preprocessing, b/c = is required too. 6f58b90

also I guess they expect some preprocessing, b/c = is required too.

You can only omit the = when passing in as a single string? This seems like the real downside...we should be able to write tests using both styles since the real deal accepts both

connorjclark · 2019-05-17T18:32:06Z

@brendankenny updated

brendankenny

looks good, just some last comment feedback

lighthouse-cli/cli-flags.js

brendankenny · 2019-05-17T22:42:20Z

lighthouse-cli/cli-flags.js

+  const result = [];
+  return result.concat(...arr);
+}
+
 /**
 * @param {string=} manualArgv
 * @return {LH.CliFlags}
 */
 function getFlags(manualArgv) {
  // @ts-ignore yargs() is incorrectly typed as not returning itself


Maybe update this one as well? // @ts-ignore yargs() is incorrectly typed as not accepting a single string.

honestly why are we even using @types/yargs :)

brendankenny · 2019-05-17T22:43:23Z

lighthouse-cli/cli-flags.js

+    // since assigning back to argv as string[] would be unsound for enums,
+    // for example: output is LH.OutputMode[].
+    const input = argv[key];
+    // Existence check + convinces TS that this is an array.


"Existence check + "...something else? Or no '+' needed

lighthouse-cli/cli-flags.js

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

googlebot · 2019-05-20T21:00:38Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

cli: accept csv for array values

6b8c607

connorjclark requested review from patrickhulce and paulirish as code owners May 11, 2019 19:25

connorjclark commented May 11, 2019

View reviewed changes

lighthouse-cli/cli-flags.js Show resolved Hide resolved

dont hardcode list of array keys

9fe961a

vercel bot deployed to staging May 11, 2019 19:41 View deployment

patrickhulce reviewed May 13, 2019

View reviewed changes

connorjclark added 2 commits May 13, 2019 11:51

Revert "dont hardcode list of array keys"

35e2f01

This reverts commit 9fe961a.

add tests for csv whitelist

879b288

vercel bot deployed to staging May 13, 2019 18:52 View deployment

patrickhulce reviewed May 13, 2019

View reviewed changes

lint, move out of .check

3e3cc4c

vercel bot deployed to staging May 13, 2019 21:56 View deployment

patrickhulce approved these changes May 13, 2019

View reviewed changes

add example

ee44360

vercel bot deployed to staging May 13, 2019 22:11 View deployment

brendankenny reviewed May 15, 2019

View reviewed changes

move example down, revise param type for flatten

0d1a611

vercel bot deployed to staging May 16, 2019 19:59 View deployment

add comment

0067f00

vercel bot deployed to staging May 16, 2019 20:02 View deployment

fix run test

6f58b90

vercel bot deployed to staging May 16, 2019 20:09 View deployment

revert getFlags param change

e4da3a9

vercel bot deployed to staging May 17, 2019 02:08 View deployment

brendankenny reviewed May 17, 2019

View reviewed changes

Update lighthouse-cli/cli-flags.js

d49db90

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

vercel bot deployed to staging May 20, 2019 21:00 View deployment

connorjclark added 2 commits May 20, 2019 14:01

Merge branch 'master' into cli-csv

2402ce8

edit comments

2b46961

vercel bot deployed to staging May 20, 2019 21:04 View deployment

brendankenny approved these changes May 22, 2019

View reviewed changes

brendankenny merged commit 066984d into master May 22, 2019

brendankenny deleted the cli-csv branch May 22, 2019 22:09

paulirish pushed a commit that referenced this pull request May 28, 2019

cli(flags): accept comma-separated array values (#8933)

3a3a1b3

brendankenny mentioned this pull request Jun 27, 2019

Rendering zero categories. #8881

Closed

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: accept csv for array values #8933

cli: accept csv for array values #8933

connorjclark commented May 11, 2019 •

edited

Loading

patrickhulce left a comment

patrickhulce May 13, 2019

connorjclark May 13, 2019

patrickhulce May 13, 2019

connorjclark May 13, 2019

connorjclark May 13, 2019

patrickhulce left a comment

patrickhulce May 13, 2019

connorjclark May 13, 2019

patrickhulce left a comment

patrickhulce May 13, 2019

brendankenny May 15, 2019 •

edited

Loading

brendankenny left a comment

brendankenny May 15, 2019

connorjclark May 16, 2019

brendankenny May 16, 2019

brendankenny May 15, 2019

connorjclark May 16, 2019

brendankenny May 16, 2019

brendankenny May 15, 2019

brendankenny May 15, 2019

connorjclark May 16, 2019 •

edited

Loading

connorjclark May 16, 2019

connorjclark May 16, 2019

brendankenny May 16, 2019

connorjclark commented May 17, 2019

brendankenny left a comment

brendankenny May 17, 2019

brendankenny May 17, 2019

brendankenny May 17, 2019

googlebot commented May 20, 2019

cli: accept csv for array values #8933

cli: accept csv for array values #8933

Conversation

connorjclark commented May 11, 2019 • edited Loading

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

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

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny May 15, 2019 • 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

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

connorjclark May 16, 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

connorjclark commented May 17, 2019

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

googlebot commented May 20, 2019

connorjclark commented May 11, 2019 •

edited

Loading

brendankenny May 15, 2019 •

edited

Loading

connorjclark May 16, 2019 •

edited

Loading