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

Preload audit improvements #4425

Closed
1 of 2 tasks
paulirish opened this issue Feb 5, 2018 · 13 comments
Closed
1 of 2 tasks

Preload audit improvements #4425

paulirish opened this issue Feb 5, 2018 · 13 comments
Assignees

Comments

@paulirish
Copy link
Member

paulirish commented Feb 5, 2018

A few followups from #3450

  • Restrict results to same-origin (as a rough filter for predictable and unchanging URLs)
  • Expand which requests are critical enough to be included. (XHRs, etc)

We could do these in separate PRs.

@wardpeet
Copy link
Collaborator

wardpeet commented Feb 7, 2018

Restrict results to same-origin (as a rough filter for predictable and unchanging URLs)

same-origin? as in the origin of url? what about cdn urls? could we use the third party badges? Or just take all origins that are in the document?

Expand which requests are critical enough to be included. (XHRs, etc)

what definition do we use? Just keep requests 2 levels deep but use all requests? (xhr, images, ...)

@patrickhulce
Copy link
Collaborator

patrickhulce commented Feb 7, 2018

same-origin? as in the origin of url? what about cdn urls? could we use the third party badges? Or just take all origins that are in the document?

I vote we include all subdomains but exclude 3rd party. i.e. for cnn.com, we could flag us2.cdn.cnn.com URLs but not cnn-cdn.amazonaws.com.

It's possible they know the URL ahead of time for 3rd party but we'll flag those domains in the preconnect audit anyhow.

@paulirish
Copy link
Member Author

I vote we include all subdomains but exclude 3rd party

ya, sounds good. lets put this lil domain comparison fn in our url-shim.

@addyosmani
Copy link
Member

I vote we include all subdomains but exclude 3rd party. i.e. for cnn.com, we could flag us2.cdn.cnn.com URLs but not cnn-cdn.amazonaws.com.

I pretty much agree. Let's roll with this.

I'm moving this to P1 (can P2) so that the preload audit is as sharp as we can get it when we highlight it at events and folks try it out.

@wardpeet
Copy link
Collaborator

wardpeet commented Jun 29, 2018

Expand which requests are critical enough to be included. (XHRs, etc)

@paulirish @patrickhulce are we still looking into this one?

@wardpeet
Copy link
Collaborator

@paulirish @patrickhulce @addyosmani @brendankenny
any feedback ? :)

@patrickhulce
Copy link
Collaborator

Whoops, sorry Ward!

Expand which requests are critical enough to be included

Seems a bit open-ended and researchy to me, but valuable for lantern too. I'm not sure if it's the highest priority thing to be doing though. Maybe as a start we see if the simplest rule "XHRs completed before FMP" actually finds anything on real sites and if it looks promising we continue?

Not sure if @addyosmani or @paulirish has other plans for you though :)

@paulirish
Copy link
Member Author

Maybe as a start we see if the simplest rule "XHRs completed before FMP" actually finds anything on real sites and if it looks promising we continue?

SG. let's.

@paulirish paulirish added P2 and removed P1 labels Aug 27, 2018
@kirill-chirkov-at-clouty

A little suggestion: on our app we have a lazy iframe, which loads some time after onload event and therefore not considered as key request for us (since it does not actually shows anything significant to user), but lighthouse detects it as key request and suggests rel=preload. I do not want to preload it since this would ruin the point of laziness itself, I wonder if this could be detected by lighthouse somehow?

@patrickhulce
Copy link
Collaborator

Thanks for adding @kirill-chirkov-at-clouty! iframes should be excluded from the preload audit, what version of Lighthouse are you using? If you're using the latest, a separate bug report with repro steps would be awesome :)

@kirill-chirkov-at-clouty
Copy link

kirill-chirkov-at-clouty commented Nov 28, 2018

@patrickhulce sure thing, I was using 3.2.1 version, I believe this is not latest for the time being (4.0.0-alpha.2-3.2.1).
UPD. Just tested with latest (4.0.0-alpha.2-3.2.1) and lazy iframe is still suggested with preload link, so guess I'm gonna create an issue then.

@paulirish
Copy link
Member Author

(3.2.1 and 4.0.0-alpha.2-3.2.1 are actually the same version, due to my mistake. :)

@paulirish
Copy link
Member Author

The remaining item from this is

Expand which requests are critical enough to be included. (XHRs, etc)

but in #11960 we determined we may be too inclusive already. i've added a brief mention of this idea to 11960 but i'll close this one now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants