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

Use preconnect resource hint instead of dns-prefetch #10780

Closed
westonruter opened this issue Dec 3, 2018 · 13 comments · Fixed by #39883
Closed

Use preconnect resource hint instead of dns-prefetch #10780

westonruter opened this issue Dec 3, 2018 · 13 comments · Fixed by #39883
Assignees
Labels
[Focus] Performance General [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Comments

@westonruter
Copy link
Contributor

When looking at the network waterfall of a Jetpack-powered site, @yoavweiss identified that the header image (served by Photon) could have been loaded earlier if there was a preconnect resource hint for i0.wp.com.

I did find that there are dns-prefetch hints for such domains:

Jetpack::dns_prefetch( array(
'//i0.wp.com',
'//i1.wp.com',
'//i2.wp.com',
) );

I see that this code was added 4 years ago, so this explains why dns-prefetch then was used is because preconnect wasn't even around at that time. (And preconnect isn't supported in IE11.)

While #10352 updates Jetpack to use wp_resource_hints (which weren't available back then either), it continues to use dns-prefetch rather than updating to use the preconnect resource hint. Should preconnect not be switched to instead?

Related: #4838, #1387.

/cc @igrigorik

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it General [Focus] Performance labels Dec 3, 2018
@jeherve
Copy link
Member

jeherve commented Dec 3, 2018

I like that suggestion, although I am not sure we should pick something that is not supported by IE11.

@dereksmart What's your take on this?

@yoavweiss
Copy link

I like that suggestion, although I am not sure we should pick something that is not supported by IE11.

To be clear, nothing will break in IE11. It'll just not perform the optimizations that other browsers would.

@ScottTravisHartley
Copy link

as was mentioned above, if you use a header preconnect, prefetch, preload and the browser doesn't support it the browser will simply skip over the tag. There is no negative for including it in this case.

However, IE 11 does support prefetch and preconnect does the DNS prefetching as well. So my question would be would the DNS prefetch simply go away, or would it stay because it's effectively doing nothing when you're using the preconnect header but if you take it away you impact performance for IE 11 users.

@yoavweiss
Copy link

Theoretically, there shouldn't be a negative effect to sending both dns-prefetch and preconnect other than some mild header bloat. At the same time, I'd carefully test it on all implementations which support preconnect before deploying it, as it could theoretically trigger side-effects (e.g. trigger 2 parallel DNS requests) which can have a negative impact, and I'm not sure all implementations tested such a scenario.

@abhinavsingi
Copy link

We shouldn't be doing preconnect everytime as it'll make connection on server and client everytime which may not be desirable for all links/etc.
dns-prefetch would be lightweight just like chrome does while typing urls in urlbar.

So when we DO neet to add preconnect if we add dns-prefetch as well then I think we'll get the best of all the browsers

@stale
Copy link

stale bot commented Aug 11, 2019

This issue has been marked as stale. This happened because:

  • It has been inactive in the past 6 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@stale stale bot added the [Status] Stale label Aug 11, 2019
@kraftbj
Copy link
Contributor

kraftbj commented Jun 26, 2020

Going to upgrade to using WP functionality in #16305. I'm open to adding in preconnect but wanted to discuss here a bit.

For Photon, it makes sense when there are images present (at least) to preconnect. The negative is extra network requests if we include it when there aren't images present. Anything else I'm missing before hacking on it a bit?

@stale stale bot removed the [Status] Stale label Jun 26, 2020
@westonruter
Copy link
Contributor Author

Can you check to see if images are likely going to be on the page before adding preconnect? Perhaps check to see if the queries post has a featured image set, or if there is a custom logo, or if there is a header image assigned, and so on?

The logic may be the same as as determining whether an image should get loading=lazy per https://core.trac.wordpress.org/ticket/50425

@airatvibe
Copy link

It is time to add preconnect for stats.wp.com and pixel.wp.com domains.
It's working faster than dns-prefetch.

IE browser is too old and not supported now.

@kraftbj
Copy link
Contributor

kraftbj commented May 31, 2021

Noting for IE, we will be deprecating IE 11 support in the next couple of months so IE is not an issue See p1HpG7-c8n-p2

@westonruter
Copy link
Contributor Author

IE 11 is history. I think this is unblocked 😄

@kraftbj
Copy link
Contributor

kraftbj commented Oct 23, 2024

Thinking aloud -- preconnect does more work than dns-prefetch. In cases like the CDNs, preconnect makes 100% sense since it is pretty expected that those domains would be used.

For cases like comments, likes, videopress shortcode, etc, just because comments are enabled, they're only loaded on posts that have commented enabled, etc.

Generally, would dns-prefect still be better to avoid the extra, possibly unneeded work unless we have it setup in a way to fire only when it is more likely to be needed (e.g. on post pages, when the shortcode is present [separate question if the work to determine that via has_shortcodes is more expensive than just preconnecting, etc], etc)?

@westonruter
Copy link
Contributor Author

@kraftbj yes, that makes sense. Ideally there should only be preconnect links when there are resources in an initial viewport that would benefit from them. I wrote #25416 (comment) on the general Jetpack Boost issue on resource preloading about how the Optimization Detective plugin facilitates this exactly. In particular, the Embed Optimizer dependent plugin leverages gathered client-side metrics to determine when an embed is in the initial viewport, and when it is, it adds preconnect links for the domains of resources known to be used by those embeds. Otherwise, if the embed is not in an initial viewport, then it goes to the opposite extreme and lazy-loads the embed's IFRAME and SCRIPT tags.

Optimization Detective and it's extension plugins—Embed Optimizer and Image Prioritizer—are part of Performance Lab from the Core Performance Team. They are being developed as feature plugins for a possible future core merge. In the meantime, perhaps Jetpack Boost could leverage them to implement performance enhancements like conditional preconnects?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Performance General [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants