-
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
core(lightwallet): add firstPartyHostnames to budget.json #10324
Conversation
This is basically a continuation of what I was previously assigned @connorjclark so I can take this one. |
Thanks for merging them @khempenius! Couple questions before I dive in...
|
@patrickhulce All good points! I'm changing course a bit and proposing what's below instead. I think this design this should answer all of the above questions. Proposed API
A hostname may optionally contain a leading Examples of valid patterns for hostnames:
Edge case example (multiple subdomains): All of these are valid inputs.
Thoughts Re: Property Name
Re: Property Value
Appendix This API assumes that granularity beyond domain-level is helpful and worth supporting and not an edge case. I see these as the two most likely use cases for this:
|
Love it! And essentially the default value for I think that addresses everything I can think of! 👍 |
Updated. If left unspecified it is |
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.
this looks fantastic!
just minor concern about the sample_v2 changes that makes me think I missed something, but LGTM other than that! 🎉
@@ -68,24 +71,31 @@ class ResourceSummary extends Audit { | |||
'third-party': str_(i18n.UIStrings.thirdPartyResourceType), | |||
}; | |||
|
|||
const types = /** @type {Array<LH.Budget.ResourceType>} */ (Object.keys(summary)); | |||
const types = /** @type {Array<Exclude<LH.Budget.ResourceType, 'third-party'>>} */ |
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.
does this change in type assertion have any effect here? just curious since it's unexpected to me :)
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.
It does - summary[type].size
and summary[type].count
will error without this change - and rightly so, since summary no longer includes a third-party
property.
assert.equal(result['third-party'].size, 0); | ||
}); | ||
const result = await ComputedResourceSummary.request(artifacts, context); | ||
assert.equal(result['third-party'], undefined); |
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.
budgets is the only place we use resource summary, right? so nothing else is affected by this change
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.
This only affects the resource-summary and performance-budget audits.
@@ -1404,7 +1403,7 @@ | |||
{ | |||
"resourceType": "image", | |||
"label": "Image", | |||
"requestCount": 2, | |||
"requestCount": 1, |
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.
The image & total request counts changed because the sample page includes an image blob & blobs are now ignored.
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.
gotcha! hurray fixes 🎉
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! 🎉
@@ -1404,7 +1403,7 @@ | |||
{ | |||
"resourceType": "image", | |||
"label": "Image", | |||
"requestCount": 2, | |||
"requestCount": 1, |
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.
gotcha! hurray fixes 🎉
Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>
…into define_3P
@khempenius is this ready to be merged? |
@patrickhulce yep, it's ready to merge :) |
Adds
options.firstPartyHostnames
tobudget.json
.API:
A hostname may optionally contain a leading
*
to match multiple subdomains as well as the root domain. For example,*.example.com
matches bothexample.com
andblog.example.com
.