core(response-compression): add transferSize sanity check #3606
Conversation
|
On second thought, I'm not completely convinced we should use transferSize everywhere either. We really want a sanity check on transferSize but still use resourceSize? Open to others thoughts |
patrickhulce
changed the title
patrickhulce
force-pushed
the
fix_response_compression
branch
from
11d32f0
to
32199c9
Compare
| @@ -21,7 +21,7 @@ class ResponsesAreCompressed extends ByteEfficiencyAudit { | |||
| */ | |||
| static get meta() { | |||
| return { | |||
| name: 'uses-request-compression', | |||
| name: 'uses-response-compression', | |||
There was a problem hiding this comment.
This is a better name, but should existing audits change names without a major version bump?
I'm admittedly completely biased because it's going to make my HTTP Archive queries (which are partly based on runner.getAuditList()) harder :)
There was a problem hiding this comment.
Yeah, that's fair, I was hoping it was new enough people weren't necessarily relying on it yet :/
patrickhulce
force-pushed
the
fix_response_compression
branch
from
32199c9
to
c1d4712
Compare
|
@brendankenny was that your only change? |
|
Hello, Than you for fix, this was quite fast! Any idea when can we expect this to work, since I'm still receiving false alert as explained in #3604 |
|
@kodi we'll hopefully be putting out a release this week, so extension and CLI by EOD Friday Chrome stable will lag behind ~2 months so end of January. |
|
Thank you, this means a lot! Again, thank you for your work (everyone)! |
| @@ -48,7 +48,8 @@ class ResponsesAreCompressed extends ByteEfficiencyAudit { | |||
| // we require at least 10% savings off the original size AND at least 1400 bytes | |||
| // if the savings is smaller than either, we don't care | |||
| if (1 - gzipSize / originalSize < IGNORE_THRESHOLD_IN_PERCENT || | |||
| gzipSavings < IGNORE_THRESHOLD_IN_BYTES | |||
| gzipSavings < IGNORE_THRESHOLD_IN_BYTES || | |||
| record.transferSize < gzipSize | |||
There was a problem hiding this comment.
Shouldn't this be a <= ?
If transferSize is exactly equal to the gzipSize then the resource should still be ignored, right?
There was a problem hiding this comment.
sure, if we want to get nitpicky about it though it should really just be
const gzipSavings = Math.min(originalSize - gzipSize, record.transferSize - gzipSize)
above but realistically this is just a sanity check that shouldn't be hit given the new mitigation in the gatherer
feel free to open a PR, we're always open to contributions :)
In other words; Lighthouse performance scores on stable are going to remain wildly incorrect for months to come. That is extremely bad news when you have to deal with tech-savvy clients that are aware of the existence of this tool inside Chrome's developer tools and like to run it themselves. I would really expect Chrome's developers to uplift a fix for this post-haste, considering the potential something like this has to caus real-world financial damage to web-developers that have to deal with clients misreading this as actually affecting their scores. (Atleast; I hope it's not actually affecting the computed score. If it is, then the problem would be considerably worse.) |
fixes #3604