-
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(lhr): include resourceSize in network-requests audit #7056
Conversation
{ | ||
key: 'resourceSize', | ||
itemType: 'bytes', | ||
displayUnit: 'kb', |
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.
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 it is, i18n lib converts it I believe
lighthouse/lighthouse-core/lib/i18n/i18n.js
Lines 130 to 134 in f4b1635
// Replace all the bytes with KB | |
parsed.elements | |
.filter(el => el.format && el.format.style === 'bytes') | |
// @ts-ignore - el.id is always defined when el.format is defined | |
.forEach(el => (clonedValues[el.id] = clonedValues[el.id] / 1024)); |
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, it's interpreted by the itemType
but is put into the LHR as the displayUnit
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.
but is put into the LHR as the displayUnit
er, that's not right. It is if it's in an i18ned string, otherwise it's not until details-renderer.js
that the displayUnit
is used...which is pointless here because these aren't displayed in the html report
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
{ | ||
key: 'resourceSize', | ||
itemType: 'bytes', | ||
displayUnit: 'kb', |
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, it's interpreted by the itemType
but is put into the LHR as the displayUnit
@@ -870,6 +880,7 @@ | |||
"startTime": 636.6400000115391, | |||
"endTime": 1213.2910000218544, | |||
"transferSize": 139, | |||
"resourceSize": 0, |
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.
why do these 0 resourceSize
items have transferSize
set?
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.
why do these 0 resourceSize items have transferSize set?
and this makes a lot more sense now when 139 isn't the transferSize
in KB, it's in bytes :)
along with the idea of #7052.... i noticed resource size isn't exposed in the lhr anywhere. seems reasonable to tack on here. wdyt?