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

Remove <link rel="preconnect dns-prefetch" href="https://www.google-analytics.com"> #1741

Closed
4 tasks done
coliff opened this issue Jun 5, 2020 · 12 comments
Closed
4 tasks done
Labels
change request Issue requests a new feature or improvement

Comments

@coliff
Copy link
Contributor

coliff commented Jun 5, 2020

I checked that...

  • ... the documentation does not mention anything about my problem
  • ... the problem doesn't occur with the default MkDocs template
  • ... the problem is not in any of my customizations (CSS, JS, template)
  • ... there are no open or closed issues that are related to my problem

Description

The theme currently has <link rel="preconnect dns-prefetch" href="https://www.google-analytics.com"> in the head. The Google Analytics integration follows immediately afterwards so there is no need to preconnect so I think this can be removed. Furthermore, I think Safari does not accept more than one value for link rel so neither would be used.
REF: https://web.dev/preconnect-and-dns-prefetch/#resolve-domain-name-early-with-reldns-prefetch

@squidfunk
Copy link
Owner

git blame shows that @Stanzilla added it. Any thoughts? I don't care what it is, but we should settle on something we all can agree on, so we won't be removing and adding it again.

@squidfunk squidfunk added the change request Issue requests a new feature or improvement label Jun 5, 2020
@Stanzilla
Copy link
Contributor

Stanzilla commented Jun 5, 2020

I would be fine with the format change but not the removal.

@coliff
Copy link
Contributor Author

coliff commented Jun 5, 2020

I can open a PR then to remove preconnect leaving <link rel="dns-prefetch" href="https://www.google-analytics.com">

@squidfunk
Copy link
Owner

squidfunk commented Jun 5, 2020

I would be fine with the format change but not the removal.

Could you elaborate? I'd be interested in the why. I'm with @coliff's objection on the location – it's basically directly in front of the actual script, so it should make no difference. Did you add this as a recommendation from Lighthouse?

@Stanzilla
Copy link
Contributor

@squidfunk yes but it should make a difference since the script is loaded with the async keyword. I'm pretty sure I compared the numbers before but I don't have them anymore.

@coliff
Copy link
Contributor Author

coliff commented Jun 5, 2020

fair enough - I'm fine to leave <link rel="dns-prefetch" href="https://www.google-analytics.com"> there then. dns-prefetch consumes less resources than preconnect and may still save a few microseconds. :-)

@Stanzilla
Copy link
Contributor

well preconnect does what dns-prefetch does + more, I only left in dns-prefetch as a fallback

@coliff
Copy link
Contributor Author

coliff commented Jun 5, 2020

I know but Google Analytics isn't a high-priority resource - it's not critical to load and display the page, by adding preconnect it can take browser resources away from loading other assets such as Google Fonts or the GitHub API (which the user does see).

https://www.webpagetest.org/result/200605_14_88ca7dd3a427cb2c9e919f74affcd297/3/details/#waterfall_view_step1

The difference either way is very small, so I don't really mind... but overall I'd suggest to not use preconnect for a non-critical resource like Google Analytics.

@squidfunk
Copy link
Owner

I missed that it's async, that's correct. However, I have to say again that I really don't want to blindly follow the Lighthouse cargo cult. I'm all for making the TTFCP, TTFMP and TTI as small as possible, but I'd say that actual measuring will lead to better decisions than following generic advice and making Lighthouse happy. In my life as a programmer, I've fallen all too often into the trap believing A is faster/better than B without having measured, and it either fired back or I was really stunned that it's not the way I expected. I'd say it's probably better too provide fewer hints to the browser than to provide too many competing hints. For this reason, my opinion is that we should remove it entirely (as suggested in the OP), unless somebody comes up with data showing that it has a measurable, significant and (most importantly) positive effect.

@squidfunk
Copy link
Owner

squidfunk commented Jun 7, 2020

Google Analytics is a non-critical and optional resource. It's loaded asynchronously, and DNS prefetching will save as little as 25-50ms (as I get from the waterfall graphs @coliff posted). Keeping the prefetching hint is a micro-optimization, which seems to have no measurable effect, but may steal resources from the browser for loading other asynchronous requests like Google Fonts or GitHub stars. For this reason, I consider it the safest to remove the link tag altogether.

We can re-consider integration when the following conditions are met:

  1. Including the tag does not slow down average render time by stealing resources.
  2. The hint has a positive impact, which for Google Analytics means a smaller rate of dropped events due to a long-running DNS request.

If you want to keep the tag, it's very easy to achieve the same with theme extension, by either extending the extrahead or analytics blocks:

{% block analytics %}
  <link rel="preconnect dns-prefetch" href="https://www.google-analytics.com">
  {{ block.super() }}
{% endblock %}

@squidfunk
Copy link
Owner

Fixed in 1df750a.

@squidfunk squidfunk added the resolved Issue is resolved, yet unreleased if open label Jun 7, 2020
@squidfunk
Copy link
Owner

Released as part of 5.2.3

@squidfunk squidfunk removed the resolved Issue is resolved, yet unreleased if open label Jun 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change request Issue requests a new feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants