-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
feat(cli): add support for custom trace categories #1866
Conversation
lighthouse-core/gather/driver.js
Outdated
*/ | ||
beginTrace(flags) { | ||
const additionalCategories = flags && flags.traceCategories && flags.traceCategories.split(','); | ||
const traceCategories = new Set(this._traceCategories.concat(additionalCategories || [])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kinda prefer moving this || []
to the above statement, so we can rely on it as an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just drop in const _uniq = arr => Array.from(new Set(arr));
and use it so we're not tossing this type from Set back to array a few lines later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lighthouse-cli/bin.ts
Outdated
@@ -78,6 +79,7 @@ const cliFlags = yargs | |||
'save-artifacts': 'Save all gathered artifacts to disk', | |||
'list-all-audits': 'Prints a list of all available audits and exits', | |||
'list-trace-categories': 'Prints a list of all required trace categories and exits', | |||
'trace-categories': 'Additional categories to capture with the trace (comma-delimited).', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about addl-trace-categories
? to clarify that this doesn't overwrite the defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additional-
but done :)
ec3ad94
to
fa2bd36
Compare
/** | ||
* @param {{additionalTraceCategories: string=}=} flags | ||
*/ | ||
beginTrace(flags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could use some fancy default params: flags = {additionalTraceCategories: ''}
then: const additionalCategories = flags.additionalTraceCategories.split(',').filter(String);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, but would still need the additionalTraceCategories check so just gets rid of flags &&
in favor of = {additionalTraceCategories: ''}
gotta save those bytes you know ;)
fixes #1663