Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: update documented throttling flags #5000

Merged
merged 2 commits into from
Apr 20, 2018
Merged

Conversation

patrickhulce
Copy link
Collaborator

doing some cleanup on the remaining references to the old disable-X-throttling flags, we should probably explore an automated way of keeping those flags in sync with our SettingsJson typedefs now that all the options are available to CLI

not sure if it's really better to piecemeal update readme like this or do a pass over our docs and re-evaluate if we still want to tell people to be doing things like we are

@patrickhulce patrickhulce changed the title cli: update throttling flags cli: update documented throttling flags Apr 19, 2018
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

we should probably explore an automated way of keeping those flags in sync with our SettingsJson typedefs

some ideas:

  • on the typescript side, instead of using @types/yargs with its generic interface, we could lie to the compiler and tell it this only returns our settings object based on the inputs, then we could do some checking... (e.g. describe first arguments all come from the settings, boolean() values do too and all have to be boolean, etc)
  • we could do a sample_v2.json-like check with for cli --help output vs the usage section in the readme . Don't know how messy it'll get with whitespace and whatnot.
  • runtime check of properties coming in as flags vs defaultSettings. No excess properties, expected types, etc. Since the defaults has the right shape checked at compile time, it can in turn ensure the right shape at run time.

@@ -50,7 +50,7 @@ Currently, `comcast` will also throttle the websocket port that Lighthouse uses
comcast --latency=150 --target-bw=1600

# Run Lighthouse with it's own throttling disabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will you fix the it's while youre in here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha done

@@ -76,7 +76,7 @@ instance with an open debugging port.

Lighthouse can run against a real mobile device. You can follow the [Remote Debugging on Android (Legacy Workflow)](https://developer.chrome.com/devtools/docs/remote-debugging-legacy) up through step 3.3, but the TL;DR is install & run adb, enable USB debugging, then port forward 9222 from the device to the machine with Lighthouse.

You'll likely want to use the CLI flags `--disable-device-emulation --disable-cpu-throttling` and potentially `--disable-network-throttling`.
You'll likely want to use the CLI flags `--disable-device-emulation --throttling.cpuSlowdownMultiplier`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update example below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -33,8 +33,8 @@ function getFlags(manualArgv) {
'lighthouse <url> --output=json --output-path=./report.json --save-assets',
'Save trace, screenshots, and named JSON report.')
.example(
'lighthouse <url> --disable-device-emulation --disable-network-throttling',
'Disable device emulation')
'lighthouse <url> --disable-device-emulation --throttling-method=provided',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts for another day: --throttling-method=provided -- this is kind of unfortunate wording if you really do just want throttling off. We could maybe make none or something that's basically like provided, though we probably don't want to make it a strict alias...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I also think this is awkward. Now with our laternification do we need our user to inform us if the throttling is off or provided? If not, then I think we should rename provided (and maybe even throttling-method).

Either way it's possible we have to introduce some CLI sugar that may be inconsistent with the config..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree it's odd, there's no difference between off and provided I'm fine to rename to whatever in followup 👍

@patrickhulce patrickhulce merged commit 70f9ecc into master Apr 20, 2018
@patrickhulce patrickhulce deleted the cleanup_cli_flags branch April 20, 2018 18:08
kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants