deps: update yargs to latest #11794

brendankenny · 2020-12-09T04:42:33Z

Two major parts of the update:

API updates. The biggest thing is that .boolean() no longer defaults to false, it defaults to undefined, while we've always relied on the default false. Also a bunch of other small improvements, like .number() and --version taking care of itself, etc
Type updates. @types/yargs has gotten super sophisticated, accumulating type info in each step of the cli flag declarations

The combo of the two started forcing more options (e.g. .boolean('flag').default('flag', false) or .option('flag', {type: 'boolean', default: false}), then arrays+strings and choices and descriptions...I eventually just opted to switch everything over to the options({flag1: opts, flag2: opts, ...}) format, which seems like a popular pattern looking at other projects.

DefinitelyTyped is limited to TypeScript 3.2 currently (and to < 4.0 until mid 2022), so types there can't do any manipulation with type names, like representing how yargs turns --some-flag into property someFlag...but we aren't limited by that :D This also adds a type transformation to model yarg's behavior. Originally I was going to augment the @types/yargs type to do that but the way yargs is set up it sends the compiler into an infinite loop before it cuts itself off. As a result, it's just a cast for now.

It's fancy, but it's really just a checked version of the unchecked cast we were doing from argv to LH.CliFlags before. It's all in typescript land, no effect on what's happening at runtime. I can revert to a plain cast if folks don't like it, though.

brendankenny · 2020-12-09T04:46:29Z

lighthouse-cli/bin.js

@@ -102,21 +102,6 @@ async function begin() {
        ' Please use "--emulated-form-factor=none" instead.');
  }

-  if (typeof cliFlags.extraHeaders === 'string') {


the coerce() functions from yargs gave a perfect place to do this loading/parsing and avoid the awkward type dance. Just moved into cli-flags

brendankenny · 2020-12-09T04:46:47Z

lighthouse-cli/cli-flags.js


 /**
 * @param {string=} manualArgv
 * @return {LH.CliFlags}
 */
 function getFlags(manualArgv) {
-  // @ts-expect-error yargs() is incorrectly typed as not accepting a single string.
+  // @ts-expect-error - undocumented, but yargs() supports parsing a single `string`.


still undocumented

brendankenny · 2020-12-09T04:48:29Z

lighthouse-cli/cli-flags.js


-      // List of options
+      // Logging


the slightly weird order of options in here is due to our currently weird order of options. This cli-flags produces exactly the same --help order except --version and --help are switched, and there are more annotated [default: false] at the end of lines.

Happy to reorder.

brendankenny · 2020-12-09T04:49:43Z

lighthouse-cli/cli-flags.js

+        },
+        'enable-error-reporting': {
+          type: 'boolean',
+          default: undefined, // Explicitly `undefined` so prompted by default.


this is the case anyways now, but doesn't hurt to be explicit

brendankenny · 2020-12-09T04:51:15Z

lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js

+  // so for now cast to add yarg's camelCase properties to type.
+  const argv = /** @type {typeof rawArgv & CamelCasify<typeof rawArgv>} */ (rawArgv);
+
+  const jobs = Number.isFinite(argv.jobs) ? argv.jobs : undefined;


not a big deal, but yargs.number() is less helpful than I'd hoped. --jobs texttext gives argv = { _: [], jobs: NaN} instead of throwing an error to the user like I hoped it would...

brendankenny · 2020-12-09T04:51:49Z

lighthouse-core/scripts/compare-runs.js

@@ -27,7 +27,7 @@ const {ProgressLogger} = require('./lantern/collect/common.js');
 const LH_ROOT = `${__dirname}/../..`;
 const ROOT_OUTPUT_DIR = `${LH_ROOT}/timings-data`;

-const argv = yargs
+const rawArgv = yargs


@connorjclark I tried to minimize the changes needed in here. I don't think I changed any semantics

brendankenny · 2020-12-09T04:57:13Z

types/externs.d.ts

@@ -170,7 +198,7 @@ declare global {
      chromeIgnoreDefaultFlags: boolean;
      chromeFlags: string | string[];
      /** Output path for the generated results. */
-      outputPath: string;
+      outputPath?: string;


as far as I can tell, this has always been optional from cli-flags, and we rely on at least the falsy behavior of outputPath for report file naming, so seems ok to document the real type coming out of yargs

paulirish · 2020-12-09T16:25:30Z

@brendankenny for documentation purposes, what are the breaking changes in here?

brendankenny · 2020-12-09T16:30:01Z

@brendankenny for documentation purposes, what are the breaking changes in here?

Actually none that I know of, but my understanding from the eng meeting last week was that it's such a large jump in yargs versions that we should consider it breaking just in case.

adamraine · 2020-12-09T19:28:57Z

lighthouse-cli/cli-flags.js

-        'disable-storage-reset':
-            'Disable clearing the browser cache and other storage APIs before a run',
-        'emulated-form-factor': 'Controls the emulated device form factor (mobile vs. desktop) if not disabled',
-        'throttling-method': 'Controls throttling method',
        'throttling.rttMs': 'Controls simulated network RTT (TCP layer)',


Why does this need to stay in a describe block?

Why does this need to stay in a describe block?

because object support in yargs is half baked and I was putting off figuring out what to do with these and then I forgot to go back and do something with them :) I'll fix.

adamraine · 2020-12-09T19:42:18Z

lighthouse-cli/cli-flags.js

+  // TODO: this function does not actually verify the object type.
+  if (value === undefined) return value;
+  if (typeof value === 'object') return /** @type {LH.SharedFlagsSettings['extraHeaders']} */ (value);
+  if (typeof value !== 'string') throw new Error('asdfasf');


Descriptive error message?

adamraine · 2020-12-09T23:35:55Z

Vscode is throwing some errors because of the backticks, are you seeing them as well?

connorjclark · 2020-12-09T23:48:44Z

Make sure Code is using the TS version of the current workspace (also maybe try a yarn after you switch branches)

cmd shift p - typescript

connorjclark · 2020-12-10T22:05:16Z

lighthouse-core/scripts/compare-runs.js

@@ -120,12 +126,15 @@ function round(value) {
 * @param {number} total
 * @return {string}
 */
-function getProgressBar(i, total = argv.n * argv.urls.length) {
+function getProgressBar(i, total = argv.n * (argv.urls || []).length) {


no way to do required arrays?

no way to do required arrays?

But I think we only want a required array of urls if doing --collect? yargs.implies() can do that, I believe, but it's a little too indirect for the typescript type to infer off of. It might benefit the CLI experience, but a JS check or a default will still have to go somewhere.

I was trying to leave everything mostly untouched so it would all be recognizable when you were next back in here, but happy to commit any suggestions

connorjclark

LGTM

adamraine

LGTM

paulirish

can you also update the --help output in the readme? I think it's a little stale already.

paulirish · 2020-12-14T19:15:20Z

lighthouse-cli/cli-flags.js

+
+      // We only have the single string positional argument, the url.
+      .option('_', {
+        array: true,


just curious why is this configured as an array?

just curious why is this configured as an array?

Really just for types. _ is always an array, but setting the yargs option type to string erases the array part, this puts it back. I added a comment to make that clear.

brendankenny requested a review from a team as a code owner December 9, 2020 04:42

brendankenny requested review from adamraine and removed request for a team December 9, 2020 04:42

google-cla bot added the cla: yes label Dec 9, 2020

devtools-bot assigned adamraine Dec 9, 2020

devtools-bot added the waiting4reviewer label Dec 9, 2020

brendankenny commented Dec 9, 2020

View reviewed changes

brendankenny added 7.0 breaking labels Dec 9, 2020

patrickhulce mentioned this pull request Dec 9, 2020

core(emulation): refactor emulation settings & CLI flags #11779

Merged

adamraine reviewed Dec 9, 2020

View reviewed changes

adamraine added waiting4committer and removed waiting4reviewer labels Dec 9, 2020

brendankenny force-pushed the yargs branch from 30b1e63 to a3a5706 Compare December 10, 2020 21:51

brendankenny added waiting4reviewer and removed waiting4committer labels Dec 10, 2020

connorjclark reviewed Dec 10, 2020

View reviewed changes

adamraine approved these changes Dec 10, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Dec 14, 2020

brendankenny added 4 commits December 14, 2020 13:04

deps: update yargs to latest

8703c4b

update smokehouse-bin

446b89a

update compare-runs

18b68bd

actual error message and throttling

6280026

brendankenny force-pushed the yargs branch from a3a5706 to 6280026 Compare December 14, 2020 19:07

vercel bot deployed to Preview December 14, 2020 19:07 View deployment

paulirish approved these changes Dec 14, 2020

View reviewed changes

upadate readme

b2d462f

vercel bot deployed to Preview December 14, 2020 22:04 View deployment

brendankenny added land-when-ci-is-green and removed waiting4committer labels Dec 14, 2020

devtools-bot merged commit 0f418a8 into master Dec 14, 2020

devtools-bot deleted the yargs branch December 14, 2020 22:48

devtools-bot removed the land-when-ci-is-green label Dec 14, 2020

deps: update yargs to latest #11794

deps: update yargs to latest #11794

Uh oh!

Conversation

brendankenny commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish commented Dec 9, 2020

Uh oh!

brendankenny commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamraine commented Dec 9, 2020

Uh oh!

connorjclark commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark left a comment

Choose a reason for hiding this comment

Uh oh!

adamraine left a comment

Choose a reason for hiding this comment

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brendankenny Dec 14, 2020 •

edited

Loading