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

Add rel=preconnect link for AMP CDN #3253

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

westonruter
Copy link
Member

In a support topic I found out that Lighthouse audits are flagging the lack of rel=preconnect links for https://cdn.ampproject.org:

image

This currently is not part of the AMP optimization guide, and I thought it was unnecessary because there is already more-specific preload links, like:

<link rel="preload" as="script" href="https://cdn.ampproject.org/v0.js">

I checked with @sebastianbenz and he pointed out that not all browsers support rel=preload, so adding a rel=preconnect link would be a good thing to do.

@westonruter westonruter added this to the v1.3 milestone Sep 16, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Sep 16, 2019
$links = [
'preconnect' => [
// Include preconnect link for AMP CDN for browsers that don't support preload.
AMP_DOM_Utils::create_node(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only room for improvement here is that I'd like it if the page already had this link that it would be prevented from being duplicated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would then come from a third-party plugin/theme instead? It would be difficult to detect, it might be easier to sanitize the duplicate away instead.

Copy link
Member Author

@westonruter westonruter Sep 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially. We actually can detect because we gather up a list of all the links right here. So we could just skip adding it if it already exists.

Aside: There is also a need to remove component scripts that do not exist among $script_handles, to remove scripts that are not relevant to the page (in case someone manually adds a script which turns out to not be relevant to the current page). See #3489.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice there should not be duplicates. After thinking about it more, it seems wasteful to detect for a duplicate link[rel=preconnect] for AMP, and it doesn't hurt to have it duplicated in the first place.

@MackenzieHartung MackenzieHartung modified the milestones: v1.3, v1.3.1 Sep 18, 2019
@swissspidy swissspidy modified the milestones: v1.3.1, v1.4 Oct 17, 2019
@westonruter westonruter removed this from the v1.4 milestone Oct 17, 2019
@westonruter westonruter added this to the v1.4 milestone Oct 23, 2019
@westonruter westonruter marked this pull request as ready for review October 23, 2019 18:11
Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Ship it.

@westonruter westonruter merged commit 6520899 into develop Oct 23, 2019
@westonruter westonruter deleted the add/amp-cache-preconnect-link branch October 23, 2019 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants