-
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(image-elements): remove resourceSize property #11733
Conversation
// HOWEVER, there are some cases where an image is compressed again over the network and transfer size | ||
// is smaller (see https://github.com/GoogleChrome/lighthouse/pull/4968). | ||
// Use the min of the two numbers to be safe. | ||
const {resourceSize = 0, transferSize = 0} = networkRecord; |
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.
@patrickhulce this is still of concern ya?
I'd expect this test to fail in this PR (but it isn't).. https://github.com/GoogleChrome/lighthouse/pull/4968/files#diff-b15e1d5e110701eeaa3cf353d4108a5e61e699ae3afbcdb272c7816b978784f1R137 (although it has since been modified 5674d61#diff-b15e1d5e110701eeaa3cf353d4108a5e61e699ae3afbcdb272c7816b978784f1 ). Seems this case was missed when that test was refactored.
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.
yes we should still account for it in the optimization cases Math.min(transferSize, resourceSize)
is mostly what we want.
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.
looks good, one question about how artifacts.json was updated
@@ -16,7 +16,6 @@ const FontSize = require('./seo/font-size.js'); | |||
|
|||
/* global window, getElementsInDocument, Image, getNodeDetails, ShadowRoot */ | |||
|
|||
|
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.
changes like this make me unreasonably happy.
lighthouse-core/test/audits/byte-efficiency/uses-responsive-images-test.js
Outdated
Show resolved
Hide resolved
lighthouse-core/test/audits/byte-efficiency/uses-responsive-images-test.js
Outdated
Show resolved
Hide resolved
lighthouse-core/test/audits/byte-efficiency/uses-responsive-images-test.js
Outdated
Show resolved
Hide resolved
"src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg", | ||
"displayedWidth": 480, | ||
"displayedHeight": 57, | ||
"src": "http://localhost:59523/dobetterweb/lighthouse-480x318.jpg?iar1", |
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.
hmm, how did the port change? did you use yarn update:sample-artifacts ImageElements
to update this?
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.
Yeah i did use that 👀
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.
it's b/c of this line https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/scripts/update-report-fixtures.js#L22 . I expected it to use the same port as the static server (10200). it being random is not good (code churn, if a line changes in a commit it should be because it really changed, not because there's a random number)
Altho this script has been like this since it was introduced 3 years ago #4793 ...
these are all the ports I found in artifacts.json
localhost:10200
localhost:50114
localhost:53133
localhost:54759
localhost:54819
localhost:9876
Eh, I'll file a bug but since this has existed forever this PR doesn't have to worry about it.
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.
LGTM!
Remove resourceSize property, and depend on the image's network property instead.
Third item of #11642