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: add inline scripts to Scripts artifact #7065

Merged
merged 27 commits into from
Mar 5, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
fac5bed
core: add inline scripts to Scripts artifact (#7060)
connorjclark Jan 19, 2019
a67cccb
make Scripts artifact an array of stuffs
connorjclark Jan 22, 2019
a8debfb
update golden
connorjclark Jan 22, 2019
8535f0b
rename var. update artifacts.json
connorjclark Jan 25, 2019
a959a03
revert changes to test artifact trace/devtools log
connorjclark Jan 28, 2019
13067d4
make Scripts.requestId optional
connorjclark Jan 28, 2019
0c28f25
meta...
connorjclark Jan 28, 2019
7a2a75b
warn instead of throw error
connorjclark Jan 28, 2019
554d398
add test for missing request id script
connorjclark Jan 28, 2019
f02e0b6
update golden lhr
connorjclark Jan 28, 2019
9321d6a
fix artifacts.json, code->content
connorjclark Jan 29, 2019
0fe823a
pr changes
connorjclark Jan 29, 2019
80a062e
update tets
connorjclark Jan 29, 2019
b5a0912
refactor syntax
connorjclark Jan 29, 2019
a38a548
Merge remote-tracking branch 'origin/master' into issue-7060-inline-s…
connorjclark Jan 31, 2019
9fc4e6d
Merge branch 'master' into issue-7060-inline-scripts
connorjclark Feb 13, 2019
cc8e401
elide inline content for js minified audit, add inline attribute to s…
connorjclark Feb 27, 2019
e78d8b5
update expected url
connorjclark Feb 27, 2019
98509ba
off by one bcuz idk
connorjclark Feb 27, 2019
8610f78
fix test
connorjclark Feb 27, 2019
c01ca17
Merge branch 'master' into issue-7060-inline-scripts
connorjclark Feb 27, 2019
54f416c
reorder
connorjclark Feb 27, 2019
9abd2f9
Merge remote-tracking branch 'origin/master' into issue-7060-inline-s…
connorjclark Mar 4, 2019
5c133cb
displayUrl, todo comment, pr changes
connorjclark Mar 4, 2019
28bfe98
update type comment
connorjclark Mar 4, 2019
b9f82ae
reverse ternary
connorjclark Mar 4, 2019
0ecd754
move const outside of try block
connorjclark Mar 4, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ module.exports = [
overallSavingsBytes: '>45000',
overallSavingsMs: '>500',
items: {
length: 1,
length: 3,
Copy link
Collaborator Author

@connorjclark connorjclark Jan 19, 2019

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

Copy link
Member

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)

Copy link
Collaborator Author

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.

},
},
},
Expand Down
28 changes: 16 additions & 12 deletions lighthouse-core/audits/byte-efficiency/unminified-javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,20 +78,24 @@ class UnminifiedJavaScript extends ByteEfficiencyAudit {
const items = [];
const warnings = [];
for (const requestId of Object.keys(artifacts.Scripts)) {
const scriptContent = artifacts.Scripts[requestId];
const scriptContentEntry = artifacts.Scripts[requestId];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PSA: typescript doesn't support type guarding for expressions like

const scriptContent = Array.isArray(artifacts.Scripts[requestId]) ?
        artifacts.Scripts[requestId] : [artifacts.Scripts[requestId]];

const scriptContents = Array.isArray(scriptContentEntry) ?
scriptContentEntry : [scriptContentEntry];
const networkRecord = networkRecords.find(record => record.requestId === requestId);
if (!networkRecord || !scriptContent) continue;
if (!networkRecord || !scriptContents) continue;

try {
const result = UnminifiedJavaScript.computeWaste(scriptContent, 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}`);
for (const scriptContent of scriptContents) {
try {
const result = UnminifiedJavaScript.computeWaste(scriptContent, networkRecord);
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
// 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}`);
}
}
}

Expand Down
21 changes: 20 additions & 1 deletion lighthouse-core/gather/gatherers/scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -20,7 +22,7 @@ class Scripts extends Gatherer {
async afterPass(passContext, loadData) {
const driver = passContext.driver;

/** @type {Object<string, string>} */
/** @type {LH.Artifacts['Scripts']} */
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
const scriptContentMap = {};
const scriptRecords = loadData.networkRecords
.filter(record => record.resourceType === NetworkRequest.TYPES.Script);
Expand All @@ -34,6 +36,23 @@ class Scripts extends Gatherer {
} catch (e) {}
}

/** @type {string[]} */
const inlineScripts = await driver.evaluateAsync(`(() => {
${getElementsInDocumentString};

return getElementsInDocument('script')
.filter(meta => !meta.src && meta.text.trim())
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like this one isn't a meta ;)

.map(meta => meta.text);
})()`, {useIsolation: true});
if (inlineScripts.length) {
const mainResource = loadData.networkRecords.find(
request => URL.equalWithExcludedFragments(request.url, passContext.url));
Copy link
Member

Choose a reason for hiding this comment

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

the other instances of finding the main resource mention URL.equalWithExcludedFragments being slow and so they have a quicker startsWith first pass. Seems like this should do the same if the speed is really a concern?

Agreed it'd be much preferable to call a centralized version of this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done. after the two merge PRs merge maybe we should just move that check into URL.equalWithExcludedFragments as an early return

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should just move that check into URL.equalWithExcludedFragments as an early return

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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 requestId optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds alright, but what are these edge cases?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done. thoughts on handling reporting of unminified scripts in the JS audit? I just put ? as a placeholder url :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also, one of us should extract that function to somewhere else (whoever merges last)

Copy link
Collaborator

Choose a reason for hiding this comment

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

extract it off of network-analyzer? that actually seems like a good place for it :)

though if you mean we should finally move network-analyzer out of the depdency-graph/simulator folder then I agree with you :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

done. thoughts on handling reporting of unminified scripts in the JS audit? I just put ? as a placeholder url

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe when the fancy code snippet preview (#6901) lands we can do something cool here :D

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 having it in network-analyzer is perfect

💯 to snippets

}
scriptContentMap[mainResource.requestId] = inlineScripts;
}

return scriptContentMap;
}
}
Expand Down
2 changes: 1 addition & 1 deletion types/artifacts.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ declare global {
/** 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>;
Scripts: Record<string, string|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. */
Expand Down