-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(lightwallet): add path to budget.json #9453
core(lightwallet): add path to budget.json #9453
Conversation
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.
thanks @khempenius this looks great!
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 think this LGTM once the fixtures are reverted! 👍 (simplest way to resolve the conflicts anyhow 😃 )
…nto lighthouse_path
@patrickhulce ready for review :) |
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.
LGTM % possible foo*foo$
bug resolution
"resourceSizes": [ | ||
{ | ||
"resourceType": "total", |
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.
OK the next series of commentary is mostly for other reviewers since I was puzzled following this train...
Stage 1
I think I see what you were asking me now @khempenius sorry I didn't understand before!
since none of these changes are necessary to test out the functionality and don't result in changes to our golden report JSON fixture, we wouldn't normally check this in. they can be reverted.
Stage 2
but later in our sample JSON it looks like this was already out of date and you're just updating to match it?
Stage 3
#8870 updated the sample report and the config to these values but not the artifacts.json here, why? Well in that same PR the update sample report command was changed to pull the config directly from sample-config.js
so in effect the entire config settings object here is ignored.
Conclusion
we don't really need to update these values ever, but it doesn't seem like a terrible idea to keep them in sync so 🤷♂
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.
#8870 updated the sample report and the config to these values but not the artifacts.json here, why? Well in that same PR the update sample report command was changed to pull the config directly from
sample-config.js
so in effect the entire config settings object here is ignored.
so, it's down to the weirdness of having the same config for -G
and -A
and that we save all our settings in the artifacts even if they aren't used in -G
and we ignore them in -A
cause we use the -A
config's settings then?
It wasn't the best set of decisions when you think about it like that :) But I guess no one uses -G
/-A
but us...
so 🤷♂
👍 👍
Can I get a description of these changes in the PR desc? or a link to a bug? I think I'm out of the loop. |
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.
looks great!
"resourceSizes": [ | ||
{ | ||
"resourceType": "total", |
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.
#8870 updated the sample report and the config to these values but not the artifacts.json here, why? Well in that same PR the update sample report command was changed to pull the config directly from
sample-config.js
so in effect the entire config settings object here is ignored.
so, it's down to the weirdness of having the same config for -G
and -A
and that we save all our settings in the artifacts even if they aren't used in -G
and we ignore them in -A
cause we use the -A
config's settings then?
It wasn't the best set of decisions when you think about it like that :) But I guess no one uses -G
/-A
but us...
so 🤷♂
👍 👍
lighthouse-core/config/budget.js
Outdated
* @param {string} pattern | ||
* @return {boolean} | ||
*/ | ||
static urlMatchesPattern(url, pattern) { |
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.
LOVE the documentation and style of this function here :D
agreed, this is great
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.
⚖️⏱+ 🛣
LGTM, thanks!
The optional
path
property is used in budget.json to indicate which page(s) a budget applies to. The path property is specified using the robots.txt format.Example of usage:
In the above example, the first budget would apply to all pages except those matching
/blog*
, and the second budget would apply to all pages matching/blog*