-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
🐛 Update preconnects in amp-youtube component #39903
Conversation
thanks @westonruter |
re-running jobs so we can merge |
@westonruter do you mind rebasing and re-pushing, ill try and get the tests green but i can seem to retrigger the bundle size to get it to pass |
db0b27f
to
79d88b3
Compare
@erwinmombay done, although I've also updated it to just remove the preconnect for fonts.gstatic.com. I found that no assets from that domain are loaded at all when emulating a mobile device. Fonts are only loaded on desktop. Since most AMP pages are served to mobile, it's probably best to just omit the fonts preconnect. |
@westonruter merged |
* Update preconnects in amp-youtube component * Remove preconnect to Google Fonts since not on mobile
* Update preconnects in amp-youtube component * Remove preconnect to Google Fonts since not on mobile
I just analyzed the resources which are loaded in a YouTube embed and they include:
fonts.gstatic.com
(for the fonts used in the title)yt3.ggpht.com
(for the channel logo)i.ytimg.com
(for the poster image)www.youtube.com
(for JS and CSS assets)www.gstatic.com
(for more JS assets)Notably,
s.ytimg.com
is not among the domains that resources are loaded from, although it is still being preconnected to:amphtml/extensions/amp-youtube/0.1/amp-youtube.js
Lines 117 to 121 in dccbba6
I suggest removing this preconnect and instead replace it with a preconnect to
fonts.gstatic.com
since it is used to render the title text which is the actual LCP element (since the poster background image is excluded based on the current LCP heuristics).