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

new_audit: third party facades #11290

Merged
merged 90 commits into from
Dec 2, 2020
Merged

new_audit: third party facades #11290

merged 90 commits into from
Dec 2, 2020

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Aug 19, 2020

Audit to identify third party resources which can be lazy loaded with a facade alternative.

Doc

Latest design (as of nov 12 2020):
image

test page: https://www.smartling.com/

Notes on design:

  • Having the total transfer size / total blocking time in the same row as the facade alternative is confusing. The user may get the impression that those are the individual transfer size / blocking time for the facade not the total. It may be better to not display any totals in that row to avoid this problem. Resolved
  • It is unclear what entity each link is a part of which could be useful information. I think we could add an extra column to hold the entity name or product name to clarify the purpose of each resource. Resolved
  • What is the "Show 3rd party resources" checkbox for. Is there a way to remove it? Resolved

Other notes:

@connorjclark
Copy link
Collaborator

connorjclark commented Aug 19, 2020

Having the total transfer size / total blocking time in the same row as the facade alternative is confusing. The user may get the impression that those are the individual transfer size / blocking time for the facade not the total. It may be better to not display any totals in that row to avoid this problem.

You can accomplish this with {key: null} in the heading.

It is unclear what entity each link is a part of which could be useful information. I think we could add an extra column to hold the entity name or product name to clarify the purpose of each resource.

For the subitems, yes? +1

What is the "Show 3rd party resources" checkbox for. Is there a way to remove it?

const thirdPartyFilterAuditExclusions = [

Moving columnBlockingTime from ThirdPartySummary.UIStrings to i18n.UIStrings forced changes to ~50 locale files. Is there a way to avoid this?

It's fine. The strings won't be retranslated. It's a noop to TC, and just a little noise in the PR.


This could be made into an opportunity by extending the ByteEfficiencyAudit, and returning a wastedBytesByUrl that transforms the 3p url sizes to the size of the facades. You might choose to do that in a second pass, but if so I recommend landing this in experimental in the interim.

I'm unclear on the usage of subitems here. It seems to map a facade to a "product type", but my assumption was that facades would be made specifically for a product.

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

quick comments

lighthouse-core/audits/lazy-third-party.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lazy-third-party.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lazy-third-party.js Outdated Show resolved Hide resolved
lighthouse-core/audits/lazy-third-party.js Outdated Show resolved Hide resolved
@adamraine
Copy link
Member Author

@connorjclark The current iteration lists individual resources which are used by a certain product, some products have more than one. Additionally, some facades can be made for multiple products, React Live Chat loader can be used with 4 different live chat clients for example. However, it is unlikely a page will ever use more than one live chat client and that is the only facade which applies to multiple products for now.

Do you think listing products rather or individual resources for each facade would be better? If listing products is better then we can eliminate the subrows for now.

Co-authored-by: Connor Clark <[email protected]>
@adamraine
Copy link
Member Author

@patrickhulce
Copy link
Collaborator

Hm, @adamraine we might want to tweak how we surface the savings to avoid underreporting since the product request identification is only for the widget request itself and not everything it brings in.

Any ideas for ways we can restructure to surface all the work of the entity once a product is detected?

@adamraine
Copy link
Member Author

@patrickhulce How do you feel about incorporating any requests from the same entity which occur after the deferrable product request? Probably won't be able to use ThirdPartySummary.getSummaries() anymore because we will want to look at network requests in order.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Aug 21, 2020

How do you feel about incorporating any requests from the same entity which occur after the deferrable product request?

Sounds like a good start 👍 we can always tweak in the future as we observe its effects

Probably won't be able to use ThirdPartySummary.getSummaries() anymore because we will want to look at network requests in order.

Or could refactor it slightly to accept an optional filter?

@adamraine
Copy link
Member Author

@patrickhulce I ran into a problem that may present itself later.

If we have an entity with 2 facades for 2 different products, how can we determine which facade to list entity resources under if they do not belong to either product are fetched after the resources for both products?

We don't currently have any entities to cause this conflict but it's difficult to ignore until it occurs.

@patrickhulce
Copy link
Collaborator

It certainly could happen @adamraine but here's why I'm not too worried about it.

At least there the worst case failure mode is that we overstate the savings on each product as the cost of them both. Given that's it's likely they'd share some base anyway and it wouldn't really be possible to tease apart which is which, I'm not terribly concerned with this outcome compared to the outcome of Lighthouse telling a user to use this facade to save 0 KiB when the cost is ~400 KiB and a ton of script execution. (sidenote how is it possible that even a single intercom widget script is 0 KiB that seems like a bug?)

WDYT?

@adamraine
Copy link
Member Author

At least there the worst case failure mode is that we overstate the savings on each product as the cost of them both.

I see your point, this is probably the best direction to go for now. The only other alternative would be to eliminate the impact numbers entirely.

sidenote how is it possible that even a single intercom widget script is 0 KiB that seems like a bug?

It looks like the transfer size is ~240 bytes which is 0.240 KiB which gets rounded down to 0 KiB. The widget.intercom.io link redirects to js.intercomcdn.com. Perhaps we should just remove the widget.intercomcdn.io from ThirdPartyWeb?

@adamraine
Copy link
Member Author

@brendankenny @patrickhulce @connorjclark Thanks for all the comments! There were a lot of them so could ya'll take another look to make sure I properly addressed your concerns :).

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.

still LGTM!

lighthouse-core/audits/third-party-facades.js Outdated Show resolved Hide resolved
@patrickhulce patrickhulce removed their assignment Nov 11, 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.

8 participants