-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(total-byte-weight): count partially finished requests #12665
Conversation
@@ -115,7 +115,8 @@ function getResponseReceivedEvent(networkRecord, index) { | |||
connectionId: networkRecord.connectionId || 140, | |||
fromDiskCache: networkRecord.fromDiskCache || false, | |||
fromServiceWorker: networkRecord.fetchedViaServiceWorker || false, | |||
encodedDataLength: networkRecord.transferSize || 0, | |||
encodedDataLength: typeof networkRecord.transferSize === 'undefined' ? |
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.
I didn't understand the note about undefined
. Why does this need to change?
Don't need to do typeof here, just networkRecord.transferSize === undefined
works. Or if the data type is somehow uncertain, flip the logic to typeof networkRecord.transferSize === 'number' ? networkRecord.transferSize : 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.
Don't need to do typeof here
ah yes, old habits :)
Or if the data type is somehow uncertain, flip the logic to
The spirit of the original I assumed was ?? 0
, i.e. use 0
as a default if some value is not explicitly set. That seems better than ignoring all values that aren't numbers.
I didn't understand the note about undefined. Why does this need to change?
If we are ever to test unexpected values coming from the protocol, we need a mechanism to mock those values in the devtools log. The affected code was attempting to handle such unexpected values.
lighthouse-core/test/audits/byte-efficiency/total-byte-weight-test.js
Outdated
Show resolved
Hide resolved
…test.js Co-authored-by: Adam Raine <[email protected]>
Summary
Removes the total byte weight policy that ignored partially downloaded resources (like videos). IMO, this dramatically understates the weight of a page in some cases (see #12664).
The check was added way back in 2017 by me as part of adding visibility into inflight records 🤯 . This was also before we used our own network request class that initializes transferSize that prevents the
undefined
concern that prompted the original.Nonetheless, I added a test to ensure spirit of the condition is still handled.
Related Issues/PRs
closes #12664