Ignoring other extension assets in request compression audit #2733
Conversation
lighthouse-core/lib/url-shim.js
Outdated
| * @return {string} | ||
| */ | ||
| URL.getProtocol = function(url) { | ||
| return new URL(url).protocol; |
There was a problem hiding this comment.
generally we've only been adding these methods to this shim if it needs a built-in try/catch. Otherwise maybe just use new URL(url).protocol directly in uses-request-compression.js
| @@ -40,8 +41,12 @@ class ResponsesAreCompressed extends ByteEfficiencyAudit { | |||
| static audit_(artifacts) { | |||
| const uncompressedResponses = artifacts.ResponseCompression; | |||
|
|
|||
| function skipUnrelatedResponses(record) { | |||
| return URL.getProtocol(record.url) !== CHROME_EXTENSION_PROTOCOL; | |||
There was a problem hiding this comment.
we recently discovered that calling new URL in a loop on the network records can be crazy slow due to data URIs (see #2701 for discussion), how about checking record.protocol/record.scheme or doing a simple url.startsWith if there's still some strong opinions about avoiding the network records
cc: @brendankenny
There was a problem hiding this comment.
we recently discovered that calling
new URLin a loop on the network records can be crazy slow due to data URIs
gah, oh yeah. I wonder if we can automate testing for audits that will run into this.
We could at least generate a network records fixture (or devtoolsLog now, I guess), that would make it easy to test an audit with many data URIs (and add it to a "writing an audit protips" section in CONTRIBUTING). With @paulirish's desire to individually annotate tests with timeouts, it would be a red flag if an individual test was taking more than two seconds to parse the test fixture.
There was a problem hiding this comment.
@patrickhulce That was my initial plan for that but then I discovered that url-shim and thought it may be better approach to put it in there :) As I recall there is only record.url no record.protocol so I'll go with record.url.startsWith
There was a problem hiding this comment.
Ah awesome :) you should have access to protocol if you end up moving it to the gatherer ;) #2733 (comment)
| const results = []; | ||
| uncompressedResponses.forEach(record => { | ||
| uncompressedResponses.filter(skipUnrelatedResponses).forEach(record => { |
There was a problem hiding this comment.
Now that I'm reading the audit I realize this actually just processes what's already been compressed over in the gatherer.
Let's filter it there instead so we don't waste cycles re-compressing something we don't care about. Sorry for the bad tip in the issue!
| @@ -10,9 +10,9 @@ const ResponsesAreCompressedAudit = | |||
| require('../../../audits/byte-efficiency/uses-request-compression.js'); | |||
| const assert = require('assert'); | |||
|
|
|||
| function generateResponse(filename, type, originalSize, gzipSize) { | |||
| function generateResponse(filename, type, originalSize, gzipSize, schema = 'http:') { | |||
There was a problem hiding this comment.
nit: can probably revert this change too now
There was a problem hiding this comment.
Yep, I must have skipped that :)
|
🎉 nice job @arturmiz! |
Fixes #2696