-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
new_audit(preload-audit): Adding preload audit #3450
Conversation
Let's let this guy chill while we focus on the bootup PRs.. |
b02437a
to
018df55
Compare
018df55
to
2cb9f33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! I think #3618 could've really helped with this one in particular, seems like a great first start though!
can probably remove gcloud-credentials too :)
if ( | ||
!networkRecord._isLinkPreload && networkRecord.protocol !== 'data' | ||
) { | ||
const wastedMs = (request.endTime - request.startTime) * 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I'm not sure we can really count this entire request time as wasted, I'm not really sure of a fantastic alternative in absence of a fastmode-based approach. Maybe we can Math.min
it with the time spent delayed instead (request.startTime - mainResource.endTime
) for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% following the fast-mode part. I kinda missed that whole part..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'use strict'; | ||
|
||
module.exports = { | ||
extends: 'lighthouse:default', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a temporary config right, we can remove this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, sorry need to check my PR's a bit better!
failureDescription: 'Scripts were discovered late but fetching them earlier may have ' + | ||
'improved how quickly the page was interactive', | ||
helpText: 'Consider using <link rel=preload> to prioritize fetching late-discovered ' + | ||
'resources sooner [Learn more]().', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, it 100% explains to beginners what it does but I have no better resource
category: 'Performance', | ||
name: 'uses-rel-preload', | ||
description: 'Preload key requests', | ||
failureDescription: 'Scripts were discovered late but fetching them earlier may have ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't need a failureDescription here, scoringMode: numeric
I think is what we're looking for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scoringMode? haven't heard about that, what does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh it basically means that there's no pass/fail state it's just a numeric score :)
I can still move the page dep graph if you like :) |
32c5a74
to
8194adc
Compare
@wardpeet I wouldn't expect that smoketest to fail this test since we're trying to target resources that were requested very late but still blocking the only real common case I can think of that will fail this in Chrome is a font requested by a render-blocking stylesheet and document.write the main case I would want to catch would be font related root document<link rel="stylesheet" href="style.css"> style.css@font-face {
font-family: "Open Sans";
src: url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2"),
url("/fonts/OpenSans-Regular-webfont.woff") format("woff");
}
body {
font-family: "Open Sans";
} the main case I would want to catch for document.write would be something like level-2.js and level-3.js in the example below, but we already have a document.write audit telling you not to use it at all so not as concerned about working around document.write with preload :) root document<script src="level-1.js"></script> level-1.jsdocument.write('<script src="level-2.js"></script>') level-2.jsdocument.write('<script src="level-3.js"></script>') level-3.jsdocument.body.appendChild(createDiv('Hello!')) |
@patrickhulce thanks! implemented smoketests as you stated. What I see now is that fonts from stylesheets are loaded from the document so they are not part of the depdency tree of critical requests Looking at the page dependencygraph gatherer I have the same issue. It seems like chrome tells us that a font is loaded by the document and not by an external stylesheet so we do not consider it as preloadable. |
ae978de
to
ae68a0e
Compare
@patrickhulce @patrickhulce wasn't 100% sure what was needed here. If not mistaking we were going to do a followup PR to expand this audit to other things than critical request chains. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we'll punt the criticalness and same-origin changes for a future PR :)
few other odds and ends
}; | ||
// add url to be backwards compatible with old CRC | ||
// TODO remove in version 3.0 | ||
request.url = request._url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the purpose of this function now then? .url
is already a getter on the request object
I'm slightly nervous about doing this since I'm assuming we needed it for a reason at some point :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed removed indeed
} | ||
|
||
const request = opts.node.request; | ||
chain.request = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe that flatten function is needed over here now?
@@ -265,8 +265,10 @@ const cli = yargs | |||
'config-path': 'The path to the config JSON file', | |||
'expectations-path': 'The path to the expected audit results file', | |||
}) | |||
.array('save-assets-path') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same story about nixing this here I believe :)
initialUrl: 'http://localhost:10200/online-only.html', | ||
url: 'http://localhost:10200/online-only.html', | ||
initialUrl: 'http://localhost:10200/preload.html', | ||
url: 'http://localhost:10200/preload.html', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're changing this URL do we need the separate entry below still?
const networkRecord = request; | ||
if (!networkRecord._isLinkPreload && networkRecord.protocol !== 'data') { | ||
// calculate time between mainresource.endTime and resource start time | ||
const wastedMs = (request._startTime - mainResource._endTime) * 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh @wardpeet I meant doing
const wastedMs = Math.min(request._startTime - mainResource._endTime, request._endTime - request._startTime) * 1000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like I misread that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { | ||
category: 'Performance', | ||
name: 'uses-rel-preload', | ||
description: 'Preload key requests', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's mark this as informative: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
lighthouse-core/config/test.js
Outdated
*/ | ||
'use strict'; | ||
|
||
module.exports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we might not need this anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do it.
what's up october 2017. welcome to the ❕ⓝⓞⓦ❕
fixes #931