core(preconnect): add warning if preconnect link is not used by patrickhulce · Pull Request #6694 · GoogleChrome/lighthouse

patrickhulce · 2018-11-30T17:08:57Z

Summary
This PR adds a warning to the preconnect audit when the origin is found in a link[rel=preconnect] but still isn't being used. Usually it's because of a missing crossorigin but good to be aware of either way.

I'd also like to start ignoring origins that were discovered within ~300 ms or so of the main document since that's basically what preconnect would do anyway, but thought I'd bring that up for discussion first.

#6676 also suggested mentioning a limit on the number of preconnect origins in our help text. I think that makes sense but goes against our "it's all in the learn more link" stance. Others Ok with it?

Related Issues/PRs
fixes #5932 #6676

wardpeet · 2018-12-02T19:48:15Z

I'd also like to start ignoring origins that were discovered within ~300 ms or so of the main document since that's basically what preconnect would do anyway, but thought I'd bring that up for discussion first.

I actually like that we discard these records. Now your warning message isn't super helpful. If I read it I would add crossorigin to the preconnect tag and I'll see the same message pop up. Sometimes crossorigin helps but the urls listed in the bugs do not get fixed by slapping crossorigin on it.

patrickhulce · 2018-12-03T03:50:34Z

I actually like that we discard these records.

As in, you would like it if we started discarding ones started within the first 300 ms? :) Right now it's kinda strict at 50 ms.

Now your warning message isn't super helpful. If I read it I would add crossorigin to the preconnect tag and I'll see the same message pop up.

Good point, should we not mention it at all then just save it for the docs?

wardpeet · 2018-12-04T17:51:50Z

As in, you would like it if we started discarding ones started within the first 300 ms? :) Right now it's kinda strict at 50 ms.

not sure what the correct timing is on this :) i'm fine with whatever is correct

Good point, should we not mention it at all then just save it for the docs?

Yes indeed, maybe just point out that the preconnect fields had no effect and refer to docs

paulirish

Isn't #5932 pointing out we tell the user to preconnect even if they already are?

Seems like we have to exclude any currently preconnected origins from our recommendations, right?

paulirish · 2018-12-07T19:06:32Z

+  async afterPass(passContext) {
+    const driver = passContext.driver;
+
+    // TODO(phulce): merge this with the main resource header values too


it does seem weird to do it both ways.

instead we could stick to the DOM methods and just new URL(href, finalUrl) for the normalized href.

Or just use the JS version and ditch the other.

oh we're not doing it both ways it returns the JS way, it seems I never pushed up the removal of the dead code in case people screamed bloody murder about doing it in browser-executed js :)

patrickhulce · 2018-12-07T19:54:42Z

Isn't #5932 pointing out we tell the user to preconnect even if they already are?

Yes, but there's a common case where they have added it incorrectly so it's not being used (i.e. missing crossorigin). It seems like we have several options

Fail normally (what we do today)
Remove it from the list and don't say anything more
Remove it from the list and add a warning that we didn't see it being respected
Fail normally and add a warning that we didn't see it being respected.

This PR is 3, but sounds like you're a fan of 2?

paulirish

This PR is 3, but sounds like you're a fan of 2?

Hmmm. I guess I'm asking why we say ericbidelman.com should preconnect to https://fonts.googleapis.com.. He already has the preconnect reference (with crossorigin). And it basically looks like the preconnect works? Perhaps this line isn't being satisfied, but probably should be?

// filter out all resources where origins are already resolved
UsesRelPreconnectAudit.hasAlreadyConnectedToOrigin(record) ||

Also unfortunately our UI for warnings in opps is kinda ugly:

paulirish · 2018-12-07T21:08:28Z

+const Gatherer = require('./gatherer');
+const getElementsInDocumentString = require('../../lib/page-functions').getElementsInDocumentString;
+
+class LinkElements extends Gatherer {


should rename the file as well.

paulirish · 2018-12-07T21:08:46Z

+  async afterPass(passContext) {
+    const driver = passContext.driver;
+
+    // TODO(phulce): merge this with the main resource header values too


paulirish · 2018-12-07T21:09:40Z

+    return driver.evaluateAsync(`(() => {
+      ${getElementsInDocumentString};
+
+      return getElementsInDocument('link').map(link => {


There's probably a difference between link elems in head vs body that will be important at some point, but for now we can ignore it.

paulirish · 2018-12-07T21:10:17Z


 const Gatherer = require('../gatherer');

+// TODO(phulce): remove this in favor of LinkElements gatherer


canonical too fwiw

this is tracked by your issue now so I'll just remove anyway :)

canonical too fwiw

this is tracked by your issue now so I'll just remove anyway :)

we should split those bullet points up into the implementation steps

paulirish · 2018-12-07T21:20:37Z

    '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 on the link is a common mistake when adding preconnect links. */
+  crossoriginWarning: 'A preconnect link was found for "{securityOrigin}" but was not used.',


Let's mention crossorigin here in the warning.

I feel weird calling these links. A preconnect "reference"?

@wardpeet and I discussed and he suggested removing the mention of crossorigin, I somewhat agree and given our other positions it makes some sense to keep it in the docs

Now your warning message isn't super helpful. If I read it I would add crossorigin to the preconnect tag and I'll see the same message pop up. Sometimes crossorigin helps but the urls listed in the bugs do not get fixed by slapping crossorigin on it.

would it be terrible to do "A preconnect <link> was..." to make it clear what's being referenced?

if not going to mention crossorigin, does the string var need a new name?

"was not used" is kind of ambiguous. "was not used by the browser"? "was not able to be used by the browser"? followed? honored?

Is there any more hint we can give about what went wrong?

would it be terrible

nope sounds good

if not going to mention crossorigin, does the string var need a new name?

heh, yes, but hang on one sec

is kind of ambiguous

was not used by the browser SGTM

Is there any more hint we can give about what went wrong?

This is 99% a crossorigin problem IMO, since you and Paul both are on that side and that was my first instinct as well, I think it's worth going back :) sorry @wardpeet!

patrickhulce · 2018-12-07T21:36:00Z

He already has the preconnect reference (with crossorigin). And it basically looks like the preconnect works?

but he doesn't? :) he's preconnecting to https://fonts.gstatic.com/ not googleapis

brendankenny · 2018-12-10T20:15:17Z


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


any reason for this change?

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

brendankenny · 2018-12-10T20:21:19Z

    '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 on the link is a common mistake when adding preconnect links. */
+  crossoriginWarning: 'A preconnect link was found for "{securityOrigin}" but was not used.',


would it be terrible to do "A preconnect <link> was..." to make it clear what's being referenced?

if not going to mention crossorigin, does the string var need a new name?

"was not used" is kind of ambiguous. "was not used by the browser"? "was not able to be used by the browser"? followed? honored?

Is there any more hint we can give about what went wrong?

brendankenny · 2018-12-10T20:24:11Z

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


move these two lines down below where they're used?

brendankenny · 2018-12-10T20:29:18Z

+ */
+'use strict';
+
+const Gatherer = require('./gatherer');


I think we settled(ish) on .js going forward?

brendankenny · 2018-12-10T20:31:40Z

+          crossorigin: link.crossorigin,
+        };
+      });
+    })()`, {useIsolation: true});


a rare useIsolation sighting!

brendankenny · 2018-12-10T20:44:52Z

+          rel: link.rel,
+          href: link.href,
+          as: link.as,
+          crossorigin: link.crossorigin,


link.crossOrigin

(we probably want to go with the same camel case for the artifact property as well?)

good points, maybe I shouldn't bring in attributes I'm not using yet 😆

brendankenny · 2018-12-10T20:48:22Z

+        rel?: string
+        href?: string
+        as?: string
+        crossorigin?: string


shouldn't these all be required since the browser will normalize to at least an empty string? Or we could have the gatherer explicitly drop '' in favor of undefined.

And from the spec/a quick test, crossorigin can be null | 'anonymous' | 'use-credentials' (the browser corrects anything else to `'anonymous')

yeah, SGTM I'll change to match spec

brendankenny · 2018-12-10T20:51:40Z

          "path": "accessibility",
        },
+        Object {
+          "path": "external-resource-links",


travis failure is just needing to update this snapshot

brendankenny · 2018-12-10T20:53:01Z


 const Gatherer = require('../gatherer');

+// TODO(phulce): remove this in favor of LinkElements gatherer


canonical too fwiw

this is tracked by your issue now so I'll just remove anyway :)

we should split those bullet points up into the implementation steps

brendankenny

This seems great and love the new artifact.

Smoke testing for the gatherer will follow when the other audits are moved to use this artifact, but any chance of getting a preconnect warning smoke test as well? We have one suggested preconnect origin in preload_style.css already, maybe we could add another and a failing <link rel="preconnect"> in the preload.html test page?

patrickhulce · 2018-12-10T21:22:55Z

any chance of getting a preconnect warning smoke test as well?

this one is trickier because it needs new origins, are you onboard for introducing external internet dependencies into the local smoke tests? I was holding off because of that, but I think a google fonts smoketest for this would the ideal test case since it's also what I imagine is the most common real-world scenario

brendankenny · 2018-12-11T00:53:31Z

this one is trickier because it needs new origins, are you onboard for introducing external internet dependencies into the local smoke tests? I was holding off because of that, but I think a google fonts smoketest for this would the ideal test case since it's also what I imagine is the most common real-world scenario

ha, maybe it is time to give up on that. We do have the two static_server ports so we can technically request from a different origin (used in e.g. fonts.html -> cors-fonts.css), but that still might not be a great setup.

An external dependency is also not as big a deal as it once was for e.g. testing on a plane since yarn smoke was split up into all its different parts, since you can route around bad internet access now.

I guess the big thing would be how flakey it would end up being. Ideally google fonts will be pretty reliable?

patrickhulce · 2018-12-11T02:12:26Z

We do have the two static_server ports so we can technically request from a different origin (used in e.g. fonts.html -> cors-fonts.css), but that still might not be a great setup.

Yeah but that's the one we're already using for real so we already ran out of origins to use for warnings! 😆

An external dependency is also not as big a deal as it once was

Yeah I agree here, I'll take the plunge

brendankenny

LGTM!

over to @paulirish

brendankenny · 2018-12-12T19:56:05Z

+        rel: string
+        href: string
+        as: string
+        crossOrigin: string|null


not worth the extra specificity?

not totally sure which direction you're suggesting going :)

'anonymous'|'use-credentials'|null instead of just string|null. It might not buy us anything (maybe if we end up with a switch statement somewhere).

oh gotcha, sure!

wardpeet · 2018-12-13T06:42:06Z

👏 sweet, looking good

core(preconnect): add warning if preconnect link is not used

7b12b27

patrickhulce requested review from brendankenny, exterkamp and paulirish as code owners November 30, 2018 17:08

exterkamp reviewed Nov 30, 2018

View reviewed changes

Comment thread lighthouse-core/gather/gatherers/seo/hreflang.js Outdated

paulirish mentioned this pull request Dec 7, 2018

Gatherer/artifact cleanup #6747

Closed

6 tasks

paulirish reviewed Dec 7, 2018

View reviewed changes

patrickhulce added 4 commits December 7, 2018 13:56

remove dead code

a79eba0

standardize on *Elements

44ba7b9

remove crossorigin reference

0ead0d1

fix fixtures

785d758

paulirish requested changes Dec 7, 2018

View reviewed changes

patrickhulce added 2 commits December 7, 2018 15:38

feedback

6dc400b

config snapshot

6e87f71

brendankenny reviewed Dec 10, 2018

View reviewed changes

patrickhulce added 2 commits December 10, 2018 15:24

feedback

f79a98b

tests

754a5ba

add smoke test

d6aa15e

brendankenny approved these changes Dec 12, 2018

View reviewed changes

brendankenny assigned paulirish Dec 12, 2018

origin regex

60148cc

paulirish approved these changes Dec 12, 2018

View reviewed changes

types

b905559

patrickhulce merged commit 2f62432 into master Dec 13, 2018

patrickhulce deleted the rel_preconnect_tweaks branch December 13, 2018 14:35

paulirish mentioned this pull request Dec 17, 2018

link rel=preload warning, but link exists #6417

Closed

patrickhulce mentioned this pull request Dec 20, 2018

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

Merged


		const Gatherer = require('../gatherer');

		// TODO(phulce): remove this in favor of LinkElements gatherer

Conversation

patrickhulce commented Nov 30, 2018

Uh oh!

Uh oh!

wardpeet commented Dec 2, 2018

Uh oh!

patrickhulce commented Dec 3, 2018

Uh oh!

wardpeet commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Dec 7, 2018

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Dec 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardpeet commented Dec 4, 2018 •

edited

Loading

patrickhulce Dec 7, 2018 •

edited

Loading

brendankenny Dec 10, 2018 •

edited

Loading

brendankenny Dec 10, 2018 •

edited

Loading

brendankenny left a comment •

edited

Loading