-
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
core: add inline scripts to Scripts artifact #7065
Changes from 22 commits
fac5bed
a67cccb
a8debfb
8535f0b
a959a03
13067d4
0c28f25
7a2a75b
554d398
f02e0b6
9321d6a
0fe823a
80a062e
b5a0912
a38a548
9fc4e6d
cc8e401
e78d8b5
98509ba
8610f78
c01ca17
54f416c
9abd2f9
5c133cb
28bfe98
b9f82ae
0ecd754
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,10 +48,11 @@ class UnminifiedJavaScript extends ByteEfficiencyAudit { | |
|
||
/** | ||
* @param {string} scriptContent | ||
* @param {LH.Artifacts.NetworkRequest} networkRecord | ||
* @param {boolean} inline | ||
* @param {LH.Artifacts.NetworkRequest|undefined} networkRecord | ||
* @return {{url: string, totalBytes: number, wastedBytes: number, wastedPercent: number}} | ||
*/ | ||
static computeWaste(scriptContent, networkRecord) { | ||
static computeWaste(scriptContent, inline, networkRecord) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rather than fall into the boolean trap, what about making a |
||
const contentLength = scriptContent.length; | ||
const totalTokenLength = computeTokenLength(scriptContent); | ||
|
||
|
@@ -61,7 +62,9 @@ class UnminifiedJavaScript extends ByteEfficiencyAudit { | |
const wastedBytes = Math.round(totalBytes * wastedRatio); | ||
|
||
return { | ||
url: networkRecord.url, | ||
url: networkRecord && !inline ? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. super nit: it feels like the inline is the special case we need to check for so it reads a bit easier in my head to say an inline thing is something that's explicitly marked as such or doesn't have a network record |
||
networkRecord.url : | ||
`inline: ${scriptContent.substr(0, 40)}...`, | ||
totalBytes, | ||
wastedBytes, | ||
wastedPercent: 100 * wastedRatio, | ||
|
@@ -77,21 +80,21 @@ class UnminifiedJavaScript extends ByteEfficiencyAudit { | |
/** @type {Array<LH.Audit.ByteEfficiencyItem>} */ | ||
const items = []; | ||
const warnings = []; | ||
for (const requestId of Object.keys(artifacts.Scripts)) { | ||
const scriptContent = artifacts.Scripts[requestId]; | ||
for (const {requestId, inline, content} of artifacts.Scripts) { | ||
if (!content) continue; | ||
const networkRecord = networkRecords.find(record => record.requestId === requestId); | ||
if (!networkRecord || !scriptContent) continue; | ||
|
||
try { | ||
const result = UnminifiedJavaScript.computeWaste(scriptContent, networkRecord); | ||
const result = UnminifiedJavaScript.computeWaste(content, inline, networkRecord); | ||
// If the ratio is minimal, the file is likely already minified, so ignore it. | ||
// If the total number of bytes to be saved is quite small, it's also safe to ignore. | ||
if (result.wastedPercent < IGNORE_THRESHOLD_IN_PERCENT || | ||
result.wastedBytes < IGNORE_THRESHOLD_IN_BYTES || | ||
!Number.isFinite(result.wastedBytes)) continue; | ||
items.push(result); | ||
} catch (err) { | ||
warnings.push(`Unable to process ${networkRecord.url}: ${err.message}`); | ||
const url = networkRecord ? networkRecord.url : '?'; | ||
warnings.push(`Unable to process script ${url}: ${err.message}`); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,8 +5,11 @@ | |
*/ | ||
'use strict'; | ||
|
||
const log = require('lighthouse-logger'); | ||
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 +23,53 @@ class Scripts extends Gatherer { | |
async afterPass(passContext, loadData) { | ||
const driver = passContext.driver; | ||
|
||
/** @type {Object<string, string>} */ | ||
const scriptContentMap = {}; | ||
/** @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(script => !script.src && script.text.trim()) | ||
.map(script => script.text); | ||
})()`, {useIsolation: true}); | ||
|
||
if (inlineScripts.length) { | ||
const mainResource = loadData.networkRecords.find(request => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how many places do we figure this out now? :) |
||
passContext.url.startsWith(request.url) && | ||
URL.equalWithExcludedFragments(request.url, passContext.url)); | ||
if (!mainResource) { | ||
log.warn('Scripts', 'could not locate mainResource'); | ||
} | ||
scripts.push( | ||
...inlineScripts.map(content => { | ||
return { | ||
content, | ||
inline: true, | ||
requestId: mainResource ? mainResource.requestId : undefined, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move this out of the It definitely sucks that we have to add this for the case that should never happen but turns out does. Maybe add a comment too about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, actually, forgot about the earlier conversation. We should switch to |
||
}; | ||
}) | ||
); | ||
} | ||
|
||
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) { | ||
scriptContentMap[record.requestId] = content; | ||
scripts.push({ | ||
content, | ||
inline: false, | ||
requestId: record.requestId, | ||
}); | ||
} | ||
} catch (e) {} | ||
} | ||
|
||
return scriptContentMap; | ||
return scripts; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,35 +16,44 @@ const resourceType = 'Script'; | |
describe('Page uses optimized responses', () => { | ||
it('fails when given unminified scripts', () => { | ||
const auditResult = UnminifiedJavascriptAudit.audit_({ | ||
Scripts: { | ||
'123.1': | ||
` | ||
Scripts: [ | ||
{ | ||
requestId: '123.1', | ||
content: ` | ||
var foo = new Set(); | ||
foo.add(1); | ||
foo.add(2); | ||
|
||
if (foo.has(2)) { | ||
console.log('hello!') | ||
} | ||
`, | ||
'123.2': | ||
` | ||
`, | ||
}, | ||
{ | ||
requestId: '123.2', | ||
content: ` | ||
const foo = new Set(); | ||
foo.add(1); | ||
|
||
async function go() { | ||
await foo.has(1) | ||
console.log('yay esnext!') | ||
} | ||
`, | ||
'123.3': | ||
/* eslint-disable no-useless-escape */ | ||
`, | ||
}, | ||
{ | ||
requestId: '123.3', | ||
content: /* eslint-disable no-useless-escape */ | ||
` | ||
const foo = 1 | ||
/Edge\/\d*\.\d*/.exec('foo') | ||
`, | ||
'123.4': '#$*%dense', | ||
}, | ||
}, | ||
{ | ||
requestId: '123.4', | ||
content: '#$*%dense', | ||
}, | ||
], | ||
}, [ | ||
{requestId: '123.1', url: 'foo.js', transferSize: 20 * KB, resourceType}, | ||
{requestId: '123.2', url: 'other.js', transferSize: 50 * KB, resourceType}, | ||
|
@@ -58,30 +67,67 @@ describe('Page uses optimized responses', () => { | |
})); | ||
|
||
expect(results).toMatchObject([ | ||
{url: 'foo.js', wastedPercent: 57, wastedKB: 11}, | ||
{url: 'other.js', wastedPercent: 53, wastedKB: 27}, | ||
{url: 'foo.js', wastedPercent: 56, wastedKB: 11}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whitespace changes in the provided network record contents. the |
||
{url: 'other.js', wastedPercent: 53, wastedKB: 26}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||
{url: 'valid-ish.js', wastedPercent: 39, wastedKB: 39}, | ||
]); | ||
}); | ||
|
||
it('passes when scripts are already minified', () => { | ||
it('fails when given unminified scripts even with missing network record', () => { | ||
const auditResult = UnminifiedJavascriptAudit.audit_({ | ||
Scripts: { | ||
'123.1': | ||
'var f=new Set();f.add(1);f.add(2);if(f.has(2))console.log(1234)', | ||
'123.2': | ||
` | ||
const foo = new Set(); | ||
Scripts: [ | ||
{ | ||
requestId: '123.1', | ||
content: ` | ||
var foo = new Set(); | ||
foo.add(1); | ||
foo.add(2); | ||
|
||
async function go() { | ||
await foo.has(1) | ||
console.log('yay esnext!') | ||
if (foo.has(2)) { | ||
console.log('hello!') | ||
} | ||
`, | ||
'123.3': | ||
'for{(wtf', | ||
}, | ||
// we can't fake the size to get over the threshold w/o a network record, | ||
// so make some really big code instead | ||
var a = 0; | ||
// ${'a++;'.repeat(2000)} | ||
`, | ||
}, | ||
], | ||
}, []); | ||
|
||
assert.strictEqual(auditResult.items.length, 1); | ||
const item = auditResult.items[0]; | ||
if (!item.url.startsWith('inline: ')) { | ||
assert.fail('url should start with "inline: "'); | ||
} | ||
assert.strictEqual(Math.round(item.wastedBytes / 1024), 3); | ||
assert.strictEqual(Math.round(item.wastedPercent), 99); | ||
}); | ||
|
||
it('passes when scripts are already minified', () => { | ||
const auditResult = UnminifiedJavascriptAudit.audit_({ | ||
Scripts: [ | ||
{ | ||
requestId: '123.1', | ||
content: 'var f=new Set();f.add(1);f.add(2);if(f.has(2))console.log(1234)', | ||
}, | ||
{ | ||
requestId: '123.2', | ||
content: ` | ||
const foo = new Set(); | ||
foo.add(1); | ||
|
||
async function go() { | ||
await foo.has(1) | ||
console.log('yay esnext!') | ||
} | ||
`, | ||
}, | ||
{ | ||
requestId: '123.3', | ||
content: 'for{(wtf', | ||
}, | ||
], | ||
}, [ | ||
{requestId: '123.1', url: 'foo.js', transferSize: 20 * KB, resourceType}, | ||
{requestId: '123.2', url: 'other.js', transferSize: 3 * KB, resourceType}, // too small | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,8 +105,8 @@ declare global { | |
RobotsTxt: {status: number|null, content: string|null}; | ||
/** Set of exceptions thrown during page load. */ | ||
RuntimeExceptions: Crdp.Runtime.ExceptionThrownEvent[]; | ||
/** The content of all scripts loaded by the page, keyed by networkRecord requestId. */ | ||
Scripts: Record<string, string>; | ||
/** The content of all scripts loaded by the page, and the networkRecord requestId that loaded them. Note, HTML documents will have one entry per script tag, all with the same requestId. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say "the networkRecord requestId that contained their content" since for it very well could have been a different script that "loaded" that other script. |
||
Scripts: Array<{content: string, inline: boolean, requestId?: string}>; | ||
/** Version information for all ServiceWorkers active after the first page load. */ | ||
ServiceWorker: {versions: Crdp.ServiceWorker.ServiceWorkerVersion[], registrations: Crdp.ServiceWorker.ServiceWorkerRegistration[]}; | ||
/** The status of an offline fetch of the page's start_url. -1 and a explanation if missing or there was an error. */ | ||
|
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.
I would feel slightly better about a looser assertion here
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.
wb a new syntax like
x % 5
to indicate within +/- 5% of x?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.
Oooh I love that, how about
x +/- y
though ;)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.
+/- landed right we can update these now?