[go: nahoru, domu]

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

core(lightwallet): add firstPartyHostnames to budget.json #10324

Merged
merged 8 commits into from
Mar 3, 2020
Merged

core(lightwallet): add firstPartyHostnames to budget.json #10324

merged 8 commits into from
Mar 3, 2020

Conversation

khempenius
Copy link
Collaborator
@khempenius khempenius commented Feb 12, 2020

Adds options.firstPartyHostnames to budget.json.

API:

"options": {
  "firstPartyHostnames": ["my-site.com", "me.my-cdn.com", "*.my-site2.com"]
}

A hostname may optionally contain a leading * to match multiple subdomains as well as the root domain. For example, *.example.com matches both example.com and blog.example.com.

@patrickhulce
Copy link
Collaborator

This is basically a continuation of what I was previously assigned @connorjclark so I can take this one.

@patrickhulce patrickhulce requested review from patrickhulce and removed request for connorjclark February 12, 2020 16:10
@patrickhulce patrickhulce self-assigned this Feb 12, 2020
@patrickhulce
Copy link
Collaborator

Thanks for merging them @khempenius! Couple questions before I dive in...

  • How would one signify that they own *.my-cdn.com? By listing https://a.my-cdn.com, https://b.my-cdn.com, etc?
  • Should it be possible to opt out of the root-level first-party consideration? Say my site is on foo.wordpress.com and WordPress injects some stuff at third-party-assets.wordpress.com, would manually specifying foo.wordpress.com remove the wider net or is this purely additional? I see the property was renamed to extraFirstPartyOrigins (maybe based on my comment in the previous PR?) so curious if that policy changed or should change.
  • Is it intentional to base this on origins rather than domains? Someone following best practices probably wouldn't be affected, but there is a bit of mismatch with our default root domain check and then others needing to care about http vs. https.

@khempenius
Copy link
Collaborator Author

@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

firstPartyHostnames: ["$HOSTNAME"]

A hostname may optionally contain a leading * to match multiple subdomains as well as the root domain. For example, *.example.com matches both example.com and blog.example.com.


Examples of valid patterns for hostnames:

  • "*.example.com"
    Applies to root domain and all subdomains.

  • "example.com"
    Only applies to root domain; does not apply to any subdomains

  • "cats.example.com"
    Applies to a specific subdomain

Edge case example (multiple subdomains):

All of these are valid inputs.

  • "www.notifications.service.gov.uk"
  • service.gov.uk"
  • "*.service.gov.uk"
  • "*.notifications.service.gov.uk"

Thoughts

Re: Property Name
Alternatives considered: firstPartyDomains, firstPartyHosts

  • I think the property name firstPartyDomains would make it easier to misinterpret example.com as applying to “everything, including subdomains, within example.com”.

  • It seems reasonable to me to follow the convention set forth in Url() for host, hostname, etc. & I can’t think of a use case where you’d want to get ports mixed up in all of this - hence hostname, rather than host. That being said firstPartyHosts would be more concise.

Re: Property Value

  • This is more configurable than blanket domain-level granularity with minimal additional user effort ("*.example.com" vs. "example.com"--> two extra characters.)
  • If a user wants to exclude a subdomain, they're stuck writing out all of the other subdomains. That being said, I don't see many sites with that many 1P subdomains. I don’t think trying to support a regex format more complex that *.<$DOMAIN> is worth the effort, IMO.

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:

  • Tag managers / analytics servers located on a 1P domain: This seems like a fairly common pattern on larger sites. e.g. Your server-side tag manager is hosted at tag-manager.my-site.com; it has a small (but non-zero) client-side footprint. It seems reasonable to me that you might want to consider any overhead associated with tags as 3P.
  • Resources loaded from multi-tenant sites: e.g. you load resources from my-site.big-cdn.com; a third party on your site loads images from third-party.big-cdn.com.

@patrickhulce
Copy link
Collaborator

Love it! And essentially the default value for firstPartyHostnames if unspecified is ["*.<tld+1>"]?

I think that addresses everything I can think of! 👍

@khempenius khempenius changed the title core(lightwallet): implement extraFirstPartyOrigins core(lightwallet): add firstPartyHostnames to budget.json Feb 25, 2020
@khempenius
Copy link
Collaborator Author

Updated.

If left unspecified it is ["*.<ROOT_DOMAIN>"] & the root domain logic comes from one of the existing helper functions that is tld-aware, so it handles things like .co.uk properly.

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.

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'>>} */
Copy link
Collaborator

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 :)

Copy link
Collaborator Author

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.

lighthouse-core/computed/third-party-summary.js Outdated Show resolved Hide resolved
lighthouse-core/config/budget.js Outdated Show resolved Hide resolved
lighthouse-core/config/budget.js Outdated Show resolved Hide resolved
assert.equal(result['third-party'].size, 0);
});
const result = await ComputedResourceSummary.request(artifacts, context);
assert.equal(result['third-party'], undefined);
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

lighthouse-core/test/computed/third-party-summary-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/results/sample_v2.json Show resolved Hide resolved
lighthouse-core/test/results/sample_v2.json Show resolved Hide resolved
@@ -1404,7 +1403,7 @@
{
"resourceType": "image",
"label": "Image",
"requestCount": 2,
"requestCount": 1,
Copy link
Collaborator Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

gotcha! hurray fixes 🎉

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.

LGTM! 🎉

lighthouse-core/test/config/budget-test.js Outdated Show resolved Hide resolved
@@ -1404,7 +1403,7 @@
{
"resourceType": "image",
"label": "Image",
"requestCount": 2,
"requestCount": 1,
Copy link
Collaborator

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>
@vercel vercel bot requested a deployment to Preview March 2, 2020 16:05 Abandoned
@vercel vercel bot requested a deployment to Preview March 2, 2020 16:05 Abandoned
@patrickhulce
Copy link
Collaborator

@khempenius is this ready to be merged?

@khempenius
Copy link
Collaborator Author

@patrickhulce yep, it's ready to merge :)

@patrickhulce patrickhulce merged commit 3b9fe96 into GoogleChrome:master Mar 3, 2020
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.

None yet

3 participants