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

core(preload): warn when duplicate requests issued #6849

Merged
merged 1 commit into from
Dec 20, 2018
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 lighthouse-cli/test/fixtures/perf/level-2.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@

/* eslint-disable */

document.write('<script src="level-3.js"></script>');
document.write('<script src="/perf/level-3.js"></script>');
3 changes: 2 additions & 1 deletion lighthouse-cli/test/fixtures/perf/preload_tester.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

/* eslint-disable */

document.write('<script src="level-2.js?delay=500"></script>');
document.write('<script src="/perf/level-2.js?delay=500"></script>');
document.write('<script src="/perf/level-2.js?warning&delay=500"></script>');

// delay our preconnect-candidates so that they're not assumed to be working already
setTimeout(() => {
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-cli/test/fixtures/preload.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<html>
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
<link rel="stylesheet" href="/perf/preload_style.css" />
<!-- WARN(preload): This should be no crossorigin -->
<link rel="preload" href="/perf/level-2.js?warning&delay=500" as="script" crossorigin="use-credentials"/>
<!-- WARN(preconnect): This should be no crossorigin -->
<link rel="preconnect" href="https://fonts.googleapis.com" crossorigin="anonymous" />
<!-- PASS(preconnect): This uses crossorigin correctly -->
Expand Down
4 changes: 4 additions & 0 deletions lighthouse-cli/test/smokehouse/perf/expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ module.exports = [
'uses-rel-preload': {
score: '<1',
rawValue: '>500',
warnings: {
0: /level-2.*warning/,
length: 1,
},
details: {
items: {
length: 1,
Expand Down
31 changes: 31 additions & 0 deletions lighthouse-core/audits/uses-rel-preload.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ const UIStrings = {
/** Description of a Lighthouse audit that tells the user *why* they should preload important network requests. The associated network requests are started halfway through pageload (or later) but should be started at the beginning. This is displayed after a user expands the section to see more. No character length limits. '<link rel=preload>' is the html code the user would include in their page and shouldn't be translated. 'Learn More' becomes link text to additional documentation. */
description: 'Consider using <link rel=preload> to prioritize fetching resources that are ' +
'currently requested later in page load. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/preload).',
/** A warning message that is shown when the user tried to follow the advice of the audit, but it's not working as expected. Forgetting to set the `crossorigin` HTML attribute, or setting it to an incorrect value, on the link is a common mistake when adding preload links. */
crossoriginWarning: 'A preload <link> was found for "{preloadURL}" but was not used ' +
'by the browser. Check that you are using the `crossorigin` attribute properly.',
};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
Expand Down Expand Up @@ -60,6 +63,25 @@ class UsesRelPreloadAudit extends Audit {
return urls;
}

/**
* Finds which URLs were attempted to be preloaded, but failed to be reused and were requested again.
*
* @param {LH.Gatherer.Simulation.GraphNode} graph
* @return {Set<string>}
*/
static getURLsFailedToPreload(graph) {
/** @type {Array<LH.Artifacts.NetworkRequest>} */
const requests = [];
graph.traverse(node => node.type === 'network' && requests.push(node.record));

const preloadRequests = requests.filter(req => req.isLinkPreload);
const preloadURLs = new Set(preloadRequests.map(req => req.url));
// A failed preload attempt will manifest as a URL that was requested twice.
// Once with `isLinkPreload` AND again without `isLinkPreload`.
const failedRequests = requests.filter(req => preloadURLs.has(req.url) && !req.isLinkPreload);
return new Set(failedRequests.map(req => req.url));
}

/**
* We want to preload all first party critical requests at depth 2.
* Third party requests can be tricky to know the URL ahead of time.
Expand Down Expand Up @@ -183,6 +205,14 @@ class UsesRelPreloadAudit extends Audit {
// sort results by wastedTime DESC
results.sort((a, b) => b.wastedMs - a.wastedMs);

/** @type {Array<string>|undefined} */
let warnings;
const failedURLs = UsesRelPreloadAudit.getURLsFailedToPreload(graph);
if (failedURLs.size) {
warnings = Array.from(failedURLs)
.map(preloadURL => str_(UIStrings.crossoriginWarning, {preloadURL}));
}

/** @type {LH.Result.Audit.OpportunityDetails['headings']} */
const headings = [
{key: 'url', valueType: 'url', label: str_(i18n.UIStrings.columnURL)},
Expand All @@ -200,6 +230,7 @@ class UsesRelPreloadAudit extends Audit {
value: results,
},
details,
warnings,
};
}
}
Expand Down
4 changes: 4 additions & 0 deletions lighthouse-core/lib/i18n/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,10 @@
"message": "Preconnect to required origins",
"description": "Imperative title of a Lighthouse audit that tells the user to connect early to internet domains that will be used to load page resources. Origin is the correct term, however 'domain name' could be used if neccsesary. This is displayed in a list of audit titles that Lighthouse generates."
},
"lighthouse-core/audits/uses-rel-preload.js | crossoriginWarning": {
"message": "A preload <link> was found for \"{preloadURL}\" but was not used by the browser. Check that you are using the `crossorigin` attribute properly.",
"description": "A warning message that is shown when the user tried to follow the advice of the audit, but it's not working as expected. Forgetting to set the `crossorigin` HTML attribute, or setting it to an incorrect value, on the link is a common mistake when adding preload links."
},
"lighthouse-core/audits/uses-rel-preload.js | description": {
"message": "Consider using <link rel=preload> to prioritize fetching resources that are currently requested later in page load. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/preload).",
"description": "Description of a Lighthouse audit that tells the user *why* they should preload important network requests. The associated network requests are started halfway through pageload (or later) but should be started at the beginning. This is displayed after a user expands the section to see more. No character length limits. '<link rel=preload>' is the html code the user would include in their page and shouldn't be translated. 'Learn More' becomes link text to additional documentation."
Expand Down