Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(unused-css-rules): no more inifinity savings #9731

Merged
merged 2 commits into from Sep 26, 2019
Merged

Conversation

patrickhulce
Copy link
Collaborator

Summary
Described by brendan already in #9684 (comment)

Related Issues/PRs
fixes #9684 (and #9614 which started this for me 馃槃 )

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Did you find any clues on what's going on with those network requests? Is the protocol wrong on size or wrong on not from cache?

lighthouse-core/audits/byte-efficiency/unused-css-rules.js Outdated Show resolved Hide resolved
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Sep 25, 2019

Did you find any clues on what's going on with those network requests? Is the protocol wrong on size or wrong on not from cache?

Oh! Yeah they were all rel=prefetch links where the server responded with a 304 Not Modified. So they were initiated only with the only purpose of populating the cache but the cache was already up to date, I guess a DevTools size of 0 makes sense in that case?

Best I can tell, in Gatsby after a service worker is activated, the page prefetches all of the resources that were already on the page. I'm not sure why.

@wardpeet do you happen to know why Gatsby does this?

@connorjclark
Copy link
Collaborator

connorjclark commented Sep 25, 2019

DevTools size of 0 makes sense in that case

doesn't it also make sense for any case with an empty response body? like, an empty .js file? jw b/c I think you mentioned that zero should be impossible

@brendankenny
Copy link
Member

somehow I missed that they were 304s, so that might have been my fault for not providing the whole story :)

@brendankenny
Copy link
Member

somehow I missed that they were 304s, so that might have been my fault for not providing the whole story :)

hmm, actually both DevTools and our records show the requests as 200. In DevTools it's only once you expand the Headers sub panel that you see a 304 in the "Response Headers" section (still 200 at the top).

It looks like it's some variant of this issue: puppeteer/puppeteer#487

But the events are different than the fix was back then. Looks like the information now comes over in Network.responseReceivedExtraInfo, which we currently don't handle.

@patrickhulce
Copy link
Collaborator Author

hmm, actually both DevTools and our records show the requests as 200. In DevTools it's only once you expand the Headers sub panel that you see a 304 in the "Response Headers" section (still 200 at the top).

Hm that's not the behavior I got in DevTools, but yeah looking back our statusCode is incorrect. Nice catch on Network.responseReceivedExtraInfo! Working on a PR? :)

@patrickhulce
Copy link
Collaborator Author

doesn't it also make sense for any case with an empty response body? like, an empty .js file? jw b/c I think you mentioned that zero should be impossible

Yeah sorry 0 shouldn't be impossible in general, I meant 0 should be impossible for a root document that resulted in stuff happening :)

In general though for cached responses that were used, DevTools would still set the resourceSize to the size of the resource pulled from the cache. This is a bit odd in that the cached response doesn't seem to be used and it's like a psuedo-200 but actually 304 response?

@brendankenny
Copy link
Member

brendankenny commented Sep 25, 2019

Working on a PR? :)

does an issue count? :)

#9738

@patrickhulce
Copy link
Collaborator Author

does an issue count? :)
#9738

Haha I just came back here to update myself, 馃憤 :)

@brendankenny
Copy link
Member

hmm, actually both DevTools and our records show the requests as 200. In DevTools it's only once you expand the Headers sub panel that you see a 304 in the "Response Headers" section (still 200 at the top).

Hm that's not the behavior I got in DevTools

were you testing in incognito by any chance? I was getting only 200s in incognito (which might have something to do with the control flow of accessing the cache when in incognito?) but when I ran in a clean regular tab (from yarn chrome) I got the 304 in the response headers.

@patrickhulce
Copy link
Collaborator Author

were you testing in incognito by any chance? I was getting only 200s in incognito (which might have something to do with the control flow of acces

Ah, no I wasn't that might've been it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime error encountered: wastedBytes provided value was not a number
4 participants