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

Move hero image preload out of experimental #814

Merged
merged 4 commits into from
Jun 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/cache-list/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,5 @@
"dependencies": {
"@ampproject/toolbox-core": "^2.5.0"
},
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "d5517559fb1c2e1be4d3c3e9df756cdce82a432a"
}
2 changes: 1 addition & 1 deletion packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,5 @@
"minimist-options": "4.1.0",
"node-fetch": "2.6.0"
},
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "da4cca026111b917cd3901b4666a5997ef45a606"
}
2 changes: 1 addition & 1 deletion packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,5 @@
"cross-fetch": "3.0.4",
"lru-cache": "5.1.1"
},
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "7a674fec36dd663876907c0c9ebd332ad3fe7967"
}
2 changes: 1 addition & 1 deletion packages/cors/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,5 @@
"url": "https://github.com/ampproject/amp-toolbox/issues"
},
"homepage": "https://github.com/ampproject/amp-toolbox/tree/master/packages/cors",
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "c26612cc3d7408fdc1451bcc8e0fecd67ac9661d"
}
2 changes: 1 addition & 1 deletion packages/lighthouse-plugin-amp/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@
"peerDependencies": {
"lighthouse": ">=6.0.0"
},
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "7a674fec36dd663876907c0c9ebd332ad3fe7967"
}
2 changes: 1 addition & 1 deletion packages/linter/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
"prettier": {
"quoteProps": "consistent"
},
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e",
"gitHead": "c26612cc3d7408fdc1451bcc8e0fecd67ac9661d",
"devDependencies": {
"prettier": "2.0.5",
"tap-parser": "10.0.1",
Expand Down
2 changes: 1 addition & 1 deletion packages/optimizer-express/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,5 @@
"mime-types": "2.1.27",
"whatwg-url": "8.1.0"
},
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "da4cca026111b917cd3901b4666a5997ef45a606"
}
2 changes: 1 addition & 1 deletion packages/optimizer/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ Minifies the generated HTML output and inlined CSS.

#### `preloadHeroImage`

Auto detect hero images for amp-img, amp-iframe, amp-video, or amp-video-iframe and injects a `link rel=preload`.
Auto detect hero images for amp-img, amp-iframe, amp-video, or amp-video-iframe and injects a `link rel=preload`. Preloads will only be generated if there is no existing image preload.

- name: `preloadHeroImage`
- valid options: `[true|false]`
Expand Down
26 changes: 24 additions & 2 deletions packages/optimizer/lib/transformers/PreloadHeroImage.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ const TINY_IMG_THRESHOLD = 150;

/**
* PreloadHeroImage - This transformer identifies a hero image or
* important images on the document and attaches <link rel="preload"> element.
* important images on the document and attaches <link rel="preload"> element. It
* will only add a hero image if there is not already an existing image preload.
*
* This transformer supports the following option:
*
Expand All @@ -36,7 +37,7 @@ const TINY_IMG_THRESHOLD = 150;
class PreloadHeroImage {
constructor(config) {
this.log = config.log;
this.enabled = config.preloadHeroImage !== false && config.experimentPreloadHeroImage;
this.enabled = config.preloadHeroImage !== false;
}
async transform(root, params) {
if (!this.enabled || params.preloadHeroImage === false) {
Expand All @@ -47,6 +48,10 @@ class PreloadHeroImage {
const body = firstChildByTag(html, 'body');
if (!body || !head) return;

if (this.hasExistingImagePreload(head)) {
return;
}

const heroImage = this.findHeroImage(body);

if (!heroImage) {
Expand Down Expand Up @@ -78,6 +83,23 @@ class PreloadHeroImage {
insertAfter(head, preload, referenceNode);
}

hasExistingImagePreload(head) {
return head.children.some(this.isImagePreload);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

continuing my series of "unhelpful comments that make the code less readable"...

return [...head.children].any(this.isImagePreload)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why the destructuring? I think head.children.some(this.isImagePreload); does the trick as well (and is easy to read).


isImagePreload(node) {
if (node.tagName !== 'link') {
return false;
}
if (!hasAttribute(node, 'rel')) {
return false;
}
if (node.attribs.rel !== 'preload') {
return false;
}
return node.attribs.as === 'image';
}

findHeroImage(root) {
if (!root.tagName) {
return null;
Expand Down
2 changes: 1 addition & 1 deletion packages/optimizer/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,5 @@
"url": "https://github.com/ampproject/amp-toolbox/issues"
},
"homepage": "https://github.com/ampproject/amp-toolbox/tree/master/packages/optimizer",
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "c0ba2297de1e3cd583018880e2ea6c7fe5fd7905"
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
<link rel="preload" href="https://cdn.ampproject.org/v0.js" as="script">
<script data-auto async src="https://cdn.ampproject.org/v0.js"></script>
<script async src="https://cdn.ampproject.org/v0/amp-video-0.1.js" custom-element="amp-video"></script>
<link rel="preload" href="https://amp.dev/static/samples/img/tokyo.jpg" as="image" data-hero>
Copy link
Collaborator

Choose a reason for hiding this comment

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

just for my own edification - what is the purpose of the data attr?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For being able to track which images have been identified as hero images

<link data-auto rel="canonical" href=".">
</head>
<body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
<link rel="preload" href="https://cdn.ampproject.org/lts/v0.js" as="script">
<script data-auto async src="https://cdn.ampproject.org/lts/v0.js"></script>
<script async src="https://cdn.ampproject.org/lts/lts/v0/amp-video-0.1.js" custom-element="amp-video"></script>
<link rel="preload" href="https://amp.dev/static/samples/img/tokyo.jpg" as="image" data-hero>
<link data-auto rel="canonical" href=".">
</head>
<body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<script async custom-element="amp-experiment" src="https://cdn.ampproject.org/v0/amp-experiment-0.1.js"></script>
<script async custom-element="amp-video" src="https://cdn.ampproject.org/v0/amp-video-0.1.js"></script>
<script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.1.js"></script>
<link rel="preload" href="https://amp.dev/static/samples/img/amp.jpg" as="image" data-hero>
<link rel="canonical" href="self.html"><style amp-custom>h1{margin:16px}</style>
</head>
<body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<script async custom-element="amp-experiment" src="https://cdn.ampproject.org/lts/v0/amp-experiment-0.1.js"></script>
<script async custom-element="amp-video" src="https://cdn.ampproject.org/lts/v0/amp-video-0.1.js"></script>
<script async custom-template="amp-mustache" src="https://cdn.ampproject.org/lts/v0/amp-mustache-0.1.js"></script>
<link rel="preload" href="https://amp.dev/static/samples/img/amp.jpg" as="image" data-hero>
<link rel="canonical" href="self.html"><style amp-custom>h1{margin:16px}</style>
</head>
<body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<script async custom-element="amp-experiment" src="https://example.com/amp/rtv/123456789000000/v0/amp-experiment-0.1.js"></script>
<script async custom-element="amp-video" src="https://example.com/amp/rtv/123456789000000/v0/amp-video-0.1.js"></script>
<script async custom-template="amp-mustache" src="https://example.com/amp/rtv/123456789000000/v0/amp-mustache-0.1.js"></script>
<link rel="preload" href="https://amp.dev/static/samples/img/amp.jpg" as="image" data-hero>
<link rel="canonical" href="self.html"><style amp-custom>h1{margin:16px}</style>
<link rel="amphtml" href="/amp-version.html">
</head>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<meta data-auto name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
<link rel="preload" href="https://cdn.ampproject.org/v0.js" as="script">
<script data-auto async src="https://cdn.ampproject.org/v0.js"></script>
<link rel="preload" href="https://octodex.github.com/images/minion.png" as="image" data-hero>
<link data-auto rel="canonical" href=".">
</head>
<body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<meta data-auto name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
<link rel="preload" href="https://cdn.ampproject.org/lts/v0.js" as="script">
<script data-auto async src="https://cdn.ampproject.org/lts/v0.js"></script>
<link rel="preload" href="https://octodex.github.com/images/minion.png" as="image" data-hero>
<link data-auto rel="canonical" href=".">
</head>
<body>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<html>
<head>
<link rel="preload" href="/another-foo.png" as="image">
<script async src="https://cdn.ampproject.org/v0.js"></script>
</head>
<body>
<amp-img width="500" height="400" src="/foo.png"></amp-img>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<html>
<head>
<link rel="preload" href="/another-foo.png" as="image">
<script async src="https://cdn.ampproject.org/v0.js"></script>
</head>
<body>
<amp-img width="500" height="400" src="/foo.png"></amp-img>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<html>
<head>
<link rel="preload" href="https://cdn.ampproject.org/v0.js" as="script">
<script async src="https://cdn.ampproject.org/v0.js"></script>
<link rel="preload" href="/foo.png" as="image" data-hero>
</head>
<body>
<amp-img width="500" height="400" src="/foo.png"></amp-img>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<html>
<head>
<link rel="preload" href="https://cdn.ampproject.org/v0.js" as="script">
<script async src="https://cdn.ampproject.org/v0.js"></script>
</head>
<body>
<amp-img width="500" height="400" src="/foo.png"></amp-img>
</body>
</html>
2 changes: 1 addition & 1 deletion packages/runtime-fetch/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,5 @@
"url": "https://github.com/ampproject/amp-toolbox/issues"
},
"homepage": "https://github.com/ampproject/amp-toolbox/tree/master/packages/runtime-fetch",
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "c26612cc3d7408fdc1451bcc8e0fecd67ac9661d"
}
2 changes: 1 addition & 1 deletion packages/runtime-version/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,5 @@
"url": "https://github.com/ampproject/amp-toolbox/issues"
},
"homepage": "https://github.com/ampproject/amp-toolbox/tree/master/packages/runtime-version",
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "d5517559fb1c2e1be4d3c3e9df756cdce82a432a"
}
2 changes: 1 addition & 1 deletion packages/update-cache/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,5 @@
"@ampproject/toolbox-cache-url": "^2.3.0",
"jsrsasign": "8.0.15"
},
"gitHead": "aeceb5cb83cc111c08297415ab469696f2f7994e"
"gitHead": "c0ba2297de1e3cd583018880e2ea6c7fe5fd7905"
}