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

clients: Output setting is always an array #6403

Closed
wants to merge 6 commits into from

Conversation

exterkamp
Copy link
Member

Summary
Removed string only uses of output, changed to array only.

Related Issues/PRs
fixes: #6309

@@ -49,7 +49,7 @@ async function runLighthouseInLR(connection, url, flags, {lrDevice, categoryIDs,
}

// pre process the LHR for proto
if (flags.output === 'json' && typeof results.report === 'string') {
if (flags.output === ['json'] && typeof results.report === 'string') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

unfortunately this doesn't work, the equality operator here is checking references not value.

> const array = ['json'];
< undefined
> ['json'] === array
< false
> ['json'] == array
< false

will need to be more explicit

flags.output && flags.output.length === 1 && flags.output[0] === 'json'

Copy link
Member Author

Choose a reason for hiding this comment

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

#saving me from myself

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

probably want to cleanup the handling of non-array output in report-generator, IIRC that's the only other place we really handle it

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

gotta update the tests too :)

@exterkamp
Copy link
Member Author

You guys are re-reviewing too fast! 😃

I have to make other changes on LR before this is ready to merge. So I'll mark this as chillin' until I can coordinate with @paulirish about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

configSettings.output can be list or string
3 participants