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(preconnect): add warning if preconnect link is not used #6694

Merged
merged 12 commits into from
Dec 13, 2018
Merged
3 changes: 3 additions & 0 deletions lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -1063,6 +1063,9 @@ Object {
Object {
"path": "accessibility",
},
Object {
"path": "link-elements",
},
Object {
"path": "dobetterweb/anchors-with-no-rel-noopener",
},
Expand Down
15 changes: 12 additions & 3 deletions lighthouse-cli/test/fixtures/perf/preload_tester.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,16 @@

/* eslint-disable */

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

// load another origin
fetch('http://localhost:10503/preload.html');
// delay our preconnect-candidates so that they're not assumed to be working already
setTimeout(() => {
// load another origin
fetch('http://localhost:10503/preload.html');

// load another origin in a way where the `crossorigin` attribute matters
const link = document.createElement('link');
link.rel = 'stylesheet';
link.href = 'https://fonts.googleapis.com/css?family=Lobster%20Two';
document.head.appendChild(link);
}, 300);
5 changes: 5 additions & 0 deletions lighthouse-cli/test/fixtures/preload.html
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
<html>
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
<link rel="stylesheet" href="/perf/preload_style.css" />
<!-- WARN(preconnect): This should be no crossorigin -->
<link rel="preconnect" href="https://fonts.googleapis.com" crossorigin="anonymous" />
<!-- PASS(preconnect): This uses crossorigin correctly -->
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
<body>
There was nothing special about this site,
nothing was left to be seen,
not even a kite.
<span style="font-family: 'Lobster Two'">Lobster Two!</span>
<script src="/perf/preload_tester.js"></script>
</body>
</html>
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 @@ -47,6 +47,10 @@ module.exports = [
},
'uses-rel-preconnect': {
score: '<1',
warnings: {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
0: /fonts.googleapis/,
length: 1,
},
details: {
items: {
length: 1,
Expand Down
21 changes: 18 additions & 3 deletions lighthouse-core/audits/uses-rel-preconnect.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

const Audit = require('./audit');
const UnusedBytes = require('./byte-efficiency/byte-efficiency-audit');
const URL = require('../lib/url-shim.js');
const i18n = require('../lib/i18n/i18n.js');
const NetworkRecords = require('../computed/network-records.js');
const MainResource = require('../computed/main-resource.js');
Expand All @@ -28,6 +29,9 @@ const UIStrings = {
description:
'Consider adding preconnect or dns-prefetch resource hints to establish early ' +
`connections to important third-party origins. [Learn more](https://developers.google.com/web/fundamentals/performance/resource-prioritization#preconnect).`,
/** 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 preconnect links. */
crossoriginWarning: 'A preconnect <link> was found for "{securityOrigin}" but was not used ' +
'by the browser. Check that you are using the `crossorigin` attribute properly.',
};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
Expand All @@ -41,7 +45,7 @@ class UsesRelPreconnectAudit extends Audit {
id: 'uses-rel-preconnect',
title: str_(UIStrings.title),
description: str_(UIStrings.description),
requiredArtifacts: ['devtoolsLogs', 'URL'],
requiredArtifacts: ['devtoolsLogs', 'URL', 'LinkElements'],
scoreDisplayMode: Audit.SCORING_MODES.NUMERIC,
};
}
Expand Down Expand Up @@ -85,13 +89,14 @@ class UsesRelPreconnectAudit extends Audit {
*/
static async audit(artifacts, context) {
const devtoolsLog = artifacts.devtoolsLogs[UsesRelPreconnectAudit.DEFAULT_PASS];
const URL = artifacts.URL;
const settings = context.settings;
let maxWasted = 0;
/** @type {string[]} */
const warnings = [];

const [networkRecords, mainResource, loadSimulator] = await Promise.all([
NetworkRecords.request(devtoolsLog, context),
MainResource.request({devtoolsLog, URL}, context),
MainResource.request({devtoolsLog, URL: artifacts.URL}, context),
LoadSimulator.request({devtoolsLog, settings}, context),
]);

Expand Down Expand Up @@ -124,6 +129,9 @@ class UsesRelPreconnectAudit extends Audit {
origins.set(securityOrigin, records);
});

const preconnectLinks = artifacts.LinkElements.filter(el => el.rel === 'preconnect');
const preconnectOrigins = new Set(preconnectLinks.map(link => URL.getOrigin(link.href || '')));

/** @type {Array<{url: string, wastedMs: number}>}*/
let results = [];
origins.forEach(records => {
Expand Down Expand Up @@ -154,6 +162,12 @@ class UsesRelPreconnectAudit extends Audit {
const wastedMs = Math.min(connectionTime, timeBetweenMainResourceAndDnsStart);
if (wastedMs < IGNORE_THRESHOLD_IN_MS) return;

if (preconnectOrigins.has(securityOrigin)) {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
// Add a warning for any origin the user tried to preconnect to but failed
warnings.push(str_(UIStrings.crossoriginWarning, {securityOrigin}));
return;
}

maxWasted = Math.max(wastedMs, maxWasted);
results.push({
url: securityOrigin,
Expand Down Expand Up @@ -181,6 +195,7 @@ class UsesRelPreconnectAudit extends Audit {
extendedInfo: {
value: results,
},
warnings,
details,
};
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ const defaultConfig = {
'chrome-console-messages',
'image-usage',
'accessibility',
'link-elements',
'dobetterweb/anchors-with-no-rel-noopener',
'dobetterweb/appcache',
'dobetterweb/doctype',
Expand Down
37 changes: 37 additions & 0 deletions lighthouse-core/gather/gatherers/link-elements.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const Gatherer = require('./gatherer.js');
const getElementsInDocumentString = require('../../lib/page-functions.js').getElementsInDocumentString; // eslint-disable-line max-len

class LinkElements extends Gatherer {
/**
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts['LinkElements']>}
*/
async afterPass(passContext) {
const driver = passContext.driver;

// TODO(phulce): merge this with the main resource header values too
Copy link
Member

Choose a reason for hiding this comment

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

merge this with the main resource header values too

what does this mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you can also specify Link values in the header of the main document

Copy link
Member

@brendankenny brendankenny Dec 10, 2018

Choose a reason for hiding this comment

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

you can also specify Link values in the header of the main document

ah, I see. LinkElements is kind of a confusing name in that case, but maybe that's ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah hence why I originally went with the external resource links but I love the standardization on *Elements so much.

I can mostly justify it by saying that HTTP headers that are meant to be equivalent stand-ins for HTML elements and vice-versa are OK to be canonicalized in gatherer-land to one name :)

// We'll use evaluateAsync because the `node.getAttribute` method doesn't actually normalize
// the values like access from JavaScript does.
return driver.evaluateAsync(`(() => {
${getElementsInDocumentString};

return getElementsInDocument('link').map(link => {
return {
rel: link.rel,
href: link.href,
as: link.as,
crossOrigin: link.crossOrigin,
};
});
})()`, {useIsolation: true});
Copy link
Member

Choose a reason for hiding this comment

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

a rare useIsolation sighting!

}
}

module.exports = LinkElements;
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 @@ -751,6 +751,10 @@
"message": "User Timing marks and measures",
"description": "Descriptive title of a diagnostic audit that provides details on any timestamps generated by the page. User Timing refers to the 'User Timing API', which enables a website to record specific times as 'marks', or spans of time as 'measures'."
},
"lighthouse-core/audits/uses-rel-preconnect.js | crossoriginWarning": {
"message": "A preconnect <link> was found for \"{securityOrigin}\" 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 preconnect links."
},
"lighthouse-core/audits/uses-rel-preconnect.js | description": {
"message": "Consider adding preconnect or dns-prefetch resource hints to establish early connections to important third-party origins. [Learn more](https://developers.google.com/web/fundamentals/performance/resource-prioritization#preconnect).",
"description": "Description of a Lighthouse audit that tells the user how to connect early to third-party domains that will be used to load page resources. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation."
Expand Down
35 changes: 35 additions & 0 deletions lighthouse-core/test/audits/uses-rel-preconnect-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ describe('Performance: uses-rel-preconnect audit', () => {
},
];
const artifacts = {
LinkElements: [],
devtoolsLogs: {[UsesRelPreconnect.DEFAULT_PASS]: networkRecordsToDevtoolsLog(networkRecords)},
URL: {finalUrl: mainResource.url},
};
Expand All @@ -54,6 +55,7 @@ describe('Performance: uses-rel-preconnect audit', () => {
},
];
const artifacts = {
LinkElements: [],
devtoolsLogs: {[UsesRelPreconnect.DEFAULT_PASS]: networkRecordsToDevtoolsLog(networkRecords)},
URL: {finalUrl: mainResource.url},
};
Expand All @@ -75,6 +77,7 @@ describe('Performance: uses-rel-preconnect audit', () => {
},
];
const artifacts = {
LinkElements: [],
devtoolsLogs: {[UsesRelPreconnect.DEFAULT_PASS]: networkRecordsToDevtoolsLog(networkRecords)},
URL: {finalUrl: mainResource.url},
};
Expand Down Expand Up @@ -102,6 +105,7 @@ describe('Performance: uses-rel-preconnect audit', () => {
},
];
const artifacts = {
LinkElements: [],
devtoolsLogs: {[UsesRelPreconnect.DEFAULT_PASS]: networkRecordsToDevtoolsLog(networkRecords)},
URL: {finalUrl: mainResource.url},
};
Expand All @@ -124,6 +128,7 @@ describe('Performance: uses-rel-preconnect audit', () => {
},
];
const artifacts = {
LinkElements: [],
devtoolsLogs: {[UsesRelPreconnect.DEFAULT_PASS]: networkRecordsToDevtoolsLog(networkRecords)},
URL: {finalUrl: mainResource.url},
};
Expand All @@ -135,6 +140,34 @@ describe('Performance: uses-rel-preconnect audit', () => {
assert.equal(details.items.length, 0);
});

it(`warns when origin has preconnect directive but not used`, async () => {
const networkRecords = [
mainResource,
{
url: 'https://cdn.example.com/request',
initiator: {},
startTime: 2,
timing: {
dnsStart: 100,
connectStart: 150,
connectEnd: 300,
receiveHeadersEnd: 2.3,
},
},
];
const artifacts = {
LinkElements: [{rel: 'preconnect', href: 'https://cdn.example.com/'}],
devtoolsLogs: {[UsesRelPreconnect.DEFAULT_PASS]: networkRecordsToDevtoolsLog(networkRecords)},
URL: {finalUrl: mainResource.url},
};

const context = {settings: {}, computedCache: new Map()};
const {score, warnings} = await UsesRelPreconnect.audit(artifacts, context);
expect(score).toBe(1);
expect(warnings).toHaveLength(1);
expect(warnings[0]).toBeDisplayString(/cdn.example.com.*not used/);
});

it(`should only list an origin once`, async () => {
const networkRecords = [
mainResource,
Expand Down Expand Up @@ -162,6 +195,7 @@ describe('Performance: uses-rel-preconnect audit', () => {
},
];
const artifacts = {
LinkElements: [],
devtoolsLogs: {[UsesRelPreconnect.DEFAULT_PASS]: networkRecordsToDevtoolsLog(networkRecords)},
URL: {finalUrl: mainResource.url},
};
Expand Down Expand Up @@ -202,6 +236,7 @@ describe('Performance: uses-rel-preconnect audit', () => {
},
];
const artifacts = {
LinkElements: [],
devtoolsLogs: {[UsesRelPreconnect.DEFAULT_PASS]: networkRecordsToDevtoolsLog(networkRecords)},
URL: {finalUrl: mainResource.url},
};
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/config/default-config-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('Default Config', () => {

// And that they have the correct shape.
opportunityResults.forEach(auditResult => {
assert.strictEqual(auditResult.details.type, 'opportunity');
Copy link
Member

Choose a reason for hiding this comment

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

any reason for this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I failed this test while I was working and the error message you get is useless, this way it shows you what audit details you actually did get back so you know where to go fix

expect(auditResult).toMatchObject({details: {type: 'opportunity'}});
assert.ok(!auditResult.errorMessage, `${auditResult.id}: ${auditResult.errorMessage}`);
assert.ok(auditResult.details.overallSavingsMs !== undefined,
`${auditResult.id} has an undefined overallSavingsMs`);
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/test/results/artifacts/artifacts.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions lighthouse-core/test/results/sample_v2.json
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,7 @@
"scoreDisplayMode": "numeric",
"rawValue": 0,
"displayValue": "",
"warnings": [],
"details": {
"type": "opportunity",
"headings": [],
Expand Down
3 changes: 2 additions & 1 deletion proto/sample_v2_round_trip.json
Original file line number Diff line number Diff line change
Expand Up @@ -2409,7 +2409,8 @@
"id": "uses-rel-preconnect",
"score": 1.0,
"scoreDisplayMode": "numeric",
"title": "Preconnect to required origins"
"title": "Preconnect to required origins",
"warnings": []
},
"uses-rel-preload": {
"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).",
Expand Down
10 changes: 10 additions & 0 deletions types/artifacts.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ declare global {
DOMStats: Artifacts.DOMStats;
/** Relevant attributes and child properties of all <object>s, <embed>s and <applet>s in the page. */
EmbeddedContent: Artifacts.EmbeddedContentInfo[];
/** All the link elements on the page. */
LinkElements: Artifacts.LinkElement[];
/** Information for font faces used in the page. */
Fonts: Artifacts.Font[];
/** Information on poorly sized font usage and the text affected by it. */
Expand Down Expand Up @@ -169,6 +171,14 @@ declare global {
params: {name: string; value: string}[];
}

/** @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#Attributes */
export interface LinkElement {
rel: string
href: string
as: string
crossOrigin: 'anonymous'|'use-credentials'|null
}

export interface Font {
display: string;
family: string;
Expand Down