feat(lightwallet): Add --budgets-path & Budgets constructor

[khempenius]

Summary:
Add --budget-path & Budget class for LightWallet.

Related Issues/PRs:
Initial discussion: #7176
#8675, #8427, #8709, #8708,#8522, #8539

[patrickhulce]

lookin' really good @khempenius!

were you planning on wiring up --budgets-path in the CLI to do the actual loading in this PR?

if not, it might make sense to split a tad further into just the Budgets class with tests and then add the flag with wiring

[paulirish]

were you planning on wiring up --budgets-path in the CLI to do the actual loading in this PR?

I think it makes sense to add the CLI bits in this PR. That way the handoff of either a path or object will be clear.

[paulirish]

Very nice stuff here.

[brendankenny]

this is looking great

[brendankenny]

reminder for myself: since this won't (yet) be in the default config, we may need to add some tests to config-test.js, e.g. the idempotent test, but we can investigate that later

[patrickhulce]

almost there, very excited for budgets! 😃

[patrickhulce]

LGTM!

I'll defer to @brendankenny for the typing business

[brendankenny]

some bigger requests, but this is all looking great.

A higher-level naming poll:

• should it be settings.budgetsJSON? Feels like it should just be settings.budgets.
• less serious: the budgets, plural, thing is going to trip people up :) It totally makes sense on settings and in the config (it's an array of budgets), but I keep writing --budget-path when it's --budgets-path and wanting to refer to my site's budget (singular, though made up of multiple rules), not budgets. But I don't know how to make those two views harmonious :)

[brendankenny]

It looks like you manually added this in? It's completely amazing you figured out this file even exists :) but it will run into the same issue as over in #8536, erased the next time someone updates the sample artifacts.

This file is generated by yarn update:sample-artifacts, which calls update-report-fixtures.js to run lighthouse against dbw_tester.html.

I think making this permanent should be as easy as

• adding this object to a budget file somewhere. Since the artifacts come from dbw_tester.html, maybe it makes sense to have the budgets file in lighthouse-cli/test/fixtures/dobetterweb/budgets.json?
• adding --budgets-path=${pathToThatPlace} to the flags in update-report-fixtures.js
• running yarn update:sample-artifacts to verify that the budget does indeed end up like this in this file
• git checkout -- lighthouse-core/test/results/artifacts/* all the generated files (devtoolslog.json, this file, etc) to get back to the state in this PR because the files will have a bunch of other changes we don't want right now :)

[brendankenny]

we can also do this in a follow up if it's not actually easy to do this

[patrickhulce]

we can also do this in a follow up if it's not actually easy to do this

I'd say +1 to this. I've manually updated/undone every programmatic change to artifacts.json to avoid unnecessary changes in recent PRs, so IMO this isn't a deal breaker

[brendankenny]

SG

[khempenius]

Is this somehow related to why this branch is passing for me locally but failing remotely?

[patrickhulce]

@khempenius it's probably the master failures #8583 #7122

I believe they're linux only, is your local Mac?

[khempenius]

Ah, makes sense. Yep, it's a Mac.

[patrickhulce]

should it be settings.budgetsJSON? Feels like it should just be settings.budgets.

I have no strong feelings either way. I can see the case being made for either one, so I'll defer to those with stronger feelings :)

But I don't know how to make those two views harmonious

We start doing "did you mean" for our CLI flags? :)

[khempenius]

@brendankenny
I changed everything from budgets* to budget*. I agree on the name thing - singular seems more intuitive and just easier to keep consistent through the codebase; a budget.json having multiple sub-budgets also doesn't seem that strange to me (e.g. nobody use aconfigs.js.)

[brendankenny]

I put up a PR against this branch in https://github.com/khempenius/lighthouse/pull/1 to skip a few formatting review round trips :) Feel free to also cherry pick from it or not use it instead.

I changed everything from budgets* to budget*. I agree on the name thing - singular seems more intuitive and just easier to keep consistent through the codebase; a budget.json having multiple sub-budgets also doesn't seem that strange to me (e.g. nobody use a configs.js.)

Ah nice, this looks great.

Last naming question is for SharedFlagsSettings.budget. Typically (but not always) we use plurals for arrays of things, which helps for things like for (const budget of settings.budgets) {}. Would it be terribly inconsistent to have a SharedFlagsSettings.budget (plural) setting we loaded from a budget.json (singular) file from the --budget-path (singular) cli flag?

If it's just going to be confusing we can keep things as is (and use for (const rule of settings.budgets) {} or whatever)

[khempenius]

SharedFlagsSettings.budgets SGTM. Updated & also merged in khempenius#1 (thank you for putting that together!).

[brendankenny]

LGTM!