search

ampproject / amppackager

Tool to improve AMP URLs via Signed Exchanges
https://amp.dev/documentation/guides-and-tutorials/optimize-and-measure/signed-exchange/
Apache License 2.0
139 stars 55 forks source link

Preconnect/dns-prefetch resource hint against web.dev best practices #489

Open schlessera opened 3 years ago

schlessera commented 3 years ago

I think this combined preconnect and dns-prefetch resource hint is not always doing what it is supposed to do depending on the browser:

https://github.com/ampproject/amppackager/blob/f1d819c86e9ab7f1f1a6578b7b4017e60fa02dc8/transformer/transformers/linktag.go#L69

As far as I can tell, the dns-prefetch is meant to be the fallback in case the preconnect is not supported (as the preconnect would include a DNS resolution). However, the dns-prefetch is mentioned first, which seems odd.

Also, according to web.dev, best practice is to use two separate hints with the dns-prefetch as the second hint serving as a fallback, as using a combined hint will break in Safari: https://web.dev/preconnect-and-dns-prefetch/#resolve-domain-name-early-with-reldns-prefetch

schlessera commented 3 years ago

Related issue in Node.JS optimizer: https://github.com/ampproject/amp-toolbox/issues/960

twifkak commented 3 years ago

Sorry I missed this. For reference, in Google this was originally added in http://cl/239478133 per http://b/118606783.

Since you landed a fix on the optimizer, I think it makes sense to do the same here, and it seems safe to do so.