core(crc): exclude non network nodes from being a leaf #9801
Conversation
|
|
||
|
|
||
| it('discards datauris as non-critical', () => { | ||
| const networkRecords = mockTracingData([HIGH, HIGH, HIGH], [[0, 1], [0, 2]]); |
There was a problem hiding this comment.
network-records-to-devtools-log.js does this a lot better :)
| @@ -104,8 +104,6 @@ class UsesRelPreloadAudit extends Audit { | |||
| if (request.isLinkPreload) return false; | |||
| // It's not critical, don't recommend it. | |||
| if (!CriticalRequestChains.isCritical(request, mainResource)) return false; | |||
There was a problem hiding this comment.
isCritical should really be on NetworkRequest or something
I would be shocked if the initiator is properly attributed to a data URI. Even if it were, I would also be shocked if there were ever a situation in which a chain of critical requests came from a data URI resource. I'm not sure I can recall a time I've seen a critical stylesheet or script served as a data URI instead of just being inlined. If we're concerned about this, I'd prefer to build a separate audit that says "why on earth did you ship a critical resource that's not a font or image as a data URI" 😆 |
so I couldn't resist and just did an http archive query for this, and there are sites that do this :) Whether or not including them is a good thing on our part becomes the operative question, I guess. One example:
https://googlechrome.github.io/lighthouse/viewer/?gist=63e85fa1861ab1cc73248d12ad3d1ab7 |
|
Color me shocked 😱 C'mon livingspaces, seriously!? |
|
one way out of this: this PR is really about fixing data URLs as critical requests themselves, not if they should be part of the critical chain. We discard a critical request from the chain here if any ancestor isn't critical; we could change that to be if any ancestor isn't critical and isn't a data URL, so chains going through a data URL will be preserved, but data URL leaves won't be included as intended by this PR. |
|
I personally don't think it's worth fixing just for this visualization, but that solution works for me if y'all do. |
worth fixing which? The whole PR or my suggestion? :) |
Oh just not worth fixing the specific issue where data URL descendants disappear, so your suggestion :) PR still seems fine to me as-is coupled maybe with a new audit to tell livingspaces to stop being so cray cray |
Co-Authored-By: Patrick Hulce <[email protected]>
|
@connorjclark is this assignment meant for me to pickup the committer portion? |
|
No, you were the reviewer, but this was before git3po assigned people, so I'm just updating assignee. Next action is still Paul. |
|
#9801 (comment) SGTM. Paul, still want to do this? |
|
I updated the branch. |
|
best attempt at a commit title, anyone wanna try improving? |
|
@paulirish @brendankenny one of you should review this now that I rewrote it. |
what's funny is that it still doesn't make sense to me, but hard to argue with the tests. |
I ran LH on nytimes and noticed data uris in the crc graph:
They seem out of place.
To me, CRC is about optimizing network load, but datauris are inlined into the parent request, so I dont think it's fair to report them in here.