core: add inline scripts to Scripts artifact #7065
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ | |
|
|
||
| const Gatherer = require('./gatherer'); | ||
| const NetworkRequest = require('../../lib/network-request'); | ||
| const getElementsInDocumentString = require('../../lib/page-functions.js').getElementsInDocumentString; // eslint-disable-line max-len | ||
| const URL = require('../../lib/url-shim.js'); | ||
|
|
||
| /** | ||
| * @fileoverview Gets JavaScript file contents. | ||
|
|
@@ -20,21 +22,46 @@ class Scripts extends Gatherer { | |
| async afterPass(passContext, loadData) { | ||
| const driver = passContext.driver; | ||
|
|
||
| /** @type {LH.Artifacts['Scripts']} */ | ||
brendankenny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const scripts = []; | ||
|
|
||
| /** @type {string[]} */ | ||
| const inlineScripts = await driver.evaluateAsync(`(() => { | ||
| ${getElementsInDocumentString}; | ||
|
|
||
| return getElementsInDocument('script') | ||
| .filter(meta => !meta.src && meta.text.trim()) | ||
|
There was a problem hiding this comment. seems like this one isn't a |
||
| .map(meta => meta.text); | ||
| })()`, {useIsolation: true}); | ||
|
|
||
| if (inlineScripts.length) { | ||
| const mainResource = loadData.networkRecords.find( | ||
| request => URL.equalWithExcludedFragments(request.url, passContext.url)); | ||
|
There was a problem hiding this comment. the other instances of finding the main resource mention Agreed it'd be much preferable to call a centralized version of this. There was a problem hiding this comment. done. after the two merge PRs merge maybe we should just move that check into There was a problem hiding this comment.
yeah it's a bit different to do generically because in these cases we know for a fact there are no fragments on the network records but agreed we should not have to do these things, the URL.* methods should just be fast by default :) |
||
| if (!mainResource) { | ||
| throw new Error('could not locate mainResource'); | ||
|
There was a problem hiding this comment. I'm a teensy bit worried that this is going to start throwing errors into our perf category on edge cases we previously didn't really care about. We don't actually need a request ID for the whole thing to work. WDYT about making There was a problem hiding this comment. sounds alright, but what are these edge cases? There was a problem hiding this comment. If I knew them I would fix them :) I just meant that we have seen this error popup in peoples reports before and it's mostly 🤷♂️and move on since it wasn't that big a deal. Now it'll be a bit more user-facing. I actually had to tackle this for canonical though and think it'll be solid. https://github.com/GoogleChrome/lighthouse/pull/7080/files#diff-2113a8215d43931339dbb50287d89dcbR450 There was a problem hiding this comment. done. thoughts on handling reporting of unminified scripts in the JS audit? I just put There was a problem hiding this comment. also, one of us should extract that function to somewhere else (whoever merges last) There was a problem hiding this comment. extract it off of though if you mean we should finally move There was a problem hiding this comment.
Ideally we'd try to provide a snippet of the code like we do for CSS, but I'm OK with this for now until we see how common it is. There was a problem hiding this comment. maybe when the fancy code snippet preview (#6901) lands we can do something cool here :D There was a problem hiding this comment. yeah having it in 💯 to snippets |
||
| } | ||
| scripts.push(...inlineScripts.map(code => ({ | ||
|
There was a problem hiding this comment. we've generally preferred the explicit cool with adding an explicit |
||
| code, | ||
| requestId: mainResource.requestId, | ||
| }))); | ||
| } | ||
|
|
||
| const scriptRecords = loadData.networkRecords | ||
| .filter(record => record.resourceType === NetworkRequest.TYPES.Script); | ||
|
|
||
| for (const record of scriptRecords) { | ||
| try { | ||
| const content = await driver.getRequestContent(record.requestId); | ||
| if (content) { | ||
| scripts.push({ | ||
| code: content, | ||
| requestId: record.requestId, | ||
| }); | ||
| } | ||
| } catch (e) {} | ||
| } | ||
|
|
||
| return scripts; | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
now takes into account these two inline scripts
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.
could we assert more in here to give insight about which 3 these should be? (shouldn't be exhaustive, but presumably the sort should be stable so could do urls)
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.
done. the data was stable too so I include it.