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(tsc): make LH.Flags type correct and consistent #5849

Merged
merged 2 commits into from
Aug 17, 2018
Merged

Conversation

brendankenny
Copy link
Member

I've had a few TODOs sprinkled in the code to fix Flags type. I'm not exactly sure what I meant by that, but this PR makes sure our uses of Flags are all correct and gets to remove two places (index.js and popup.js) where we were coercing empty objects into Flags.

We allowed empty objects as flags there, and now the type system actually verifies that that's completely acceptable :)

We already had LH.Flags vs LH.SharedFlagsSettings (all properties shared by LH.Flags and LH.Config.Settings), and now I've added another level with LH.Flags vs LH.CliFlags (all properties in addition to LH.Flags that control just CLI functionality). This is another step in making Flags a little easier to reason about, because once they reach core, all those CLI flags aren't visible anymore (according to the type system). This means code really doesn't have to care if Flags originated from the CLI, someone calling the node module, the extension, or devtools.

Also spruces up type annotations in the CLI files (mostly removing !s), which haven't been touched in a while.

@brendankenny brendankenny changed the title core(tsc): make LH.Flags type correct and consistent cli(tsc): make LH.Flags type correct and consistent Aug 16, 2018
* @param {Partial<LH.Flags>=} flags
* @return {Partial<LH.Config.Settings>}
*/
* @return {RecursivePartial<LH.Config.Settings>}
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bug that this wasn't flagged before, but it wasn't until I made any change to LH.Flags.output that it started to be flagged (even though the problem is with LH.Flags.throttling, weirdly enough).

We just want to copy any Settings properties over from an instance of Flags, so the result will only have Settings properties but some may not be set. So Partial<LH.Config.Settings> would be correct if Settings were a simple object.

The issue is that one of the properties (throttling) is itself an object, and while Flags.throttling has all optional properties, on Settings.throttling they're all required. This change applies Partial<> to all of Settings properties recursively. It could special case throttling instead, but this will catch any future objects added to SharedFlagsSettings as well.

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.

seems jolly fine to me! 🎅

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

lg besides the enableErrorReporting thing

_: string[];
chromeFlags: string;
outputPath: string;
saveAssets: boolean;
view: boolean;
enableErrorReporting: boolean;
enableErrorReporting?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

make this a yargs boolean instead. seems weird to have this one optional but the rest not.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

wait noooooo, we gotta revert this. it's purposefully not a yargs boolean so that the setting saved to their config will be respected. explicitly setting as false via yargs will always turn error reporting off

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops, very good point. There was a reason it wasn't in yargs.boolean() already :) I'll revert and add a comment so we don't do that again

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.

3 participants