core(lr): add LR presets for desktop and mobile #5886
Conversation
| disableStorageReset: true, | ||
| disableDeviceEmulation: true, | ||
| throttling: { | ||
| // Using a "broadband" connection type |
There was a problem hiding this comment.
I ran a few tests with this vs. observed mode unthrottled. Observed tended to produce slightly more optimistic and variable results, so I'm inclined to say we keep this (then we should also be covered for that batch case @paulirish), but I don't feel strongly and would be fine with throttlingMethod: 'observed' as well.
| flags.logLevel = flags.logLevel || 'info'; | ||
| const config = background.getDefaultConfigForCategories(categoryIDs); | ||
| const config = LR_PRESETS[preset || 'lr-mobile']; |
There was a problem hiding this comment.
wdyt about mapping LR Device -> Preset within here?
Looks like preset would be a string of either: 'UNKNOWN_DEVICE', 'MOBILE', or 'DESKTOP'
There was a problem hiding this comment.
sure, can we name it something other than preset then :)
| flags.logLevel = flags.logLevel || 'info'; | ||
| const config = background.getDefaultConfigForCategories(categoryIDs); | ||
| const config = LR_PRESETS[preset || 'lr-mobile']; | ||
| if (categoryIDs) { |
There was a problem hiding this comment.
is it me or are you also beguiled at how this undefined string actually works.. https://goto.google.com/wmjay
maybe every caller already expresses all cats, so we just never hit it?
😕
There was a problem hiding this comment.
at how this undefined string actually works...
yeah that's super confusing, my guess is that it's invoked via the template string which will either stringify the value or return 'undefined'?
There was a problem hiding this comment.
ah the array (or undefined) is totally manually constructed. L204-218
lovely. :)
lighthouse-cli/cli-flags.js
Outdated
| @@ -121,7 +121,7 @@ function getFlags(manualArgv) { | |||
| ]) | |||
| .choices('output', printer.getValidOutputOptions()) | |||
| .choices('throttling-method', ['devtools', 'provided', 'simulate']) | |||
| .choices('preset', ['full', 'perf', 'mixed-content']) | |||
| .choices('preset', ['full', 'perf', 'mixed-content', 'lr-mobile', 'lr-desktop']) | |||
There was a problem hiding this comment.
we don't need to expose these to CLI, IMO.
let's keep em private to core.
|
@patrickhulce i notice afterwards that the desktop preset creates a report that still says "Simulated Fast 3G network" at the top. however the items in the footer are correct. |
|
@paulirish yeah but we're not using the header in LR right? didn't think we needed to make that flexible since we're officially saying do this at your own risk :) |
|
We use it when using the googleplex demo all with HTML format. But it's not a big deal to me. Just noting it. |
Oh, ok. Probably worth silently establishing the other network presets as configs and updating the header to match :) |
Introduces a desktop and mobile config preset of LR, mobile is arguably unnecessary but if LR uses it then it allows us to easily change the configuration in the future.
Also brought over a few things from the run string over on LR side to hopefully reduce the complexity there a bit. @paulirish you can comment if you'd prefer to have it here or there.