Fixing request chain #1303
Fixing request chain #1303
Conversation
|
I moved from the "chainCount" to the chain depth. I'm not sure what the benefit is of one over the other but given that every test for lighthouse expected the test to fail, now there are two tests that allow the rule to pass (the online-only.html fixture in npm run smoke and the sample valid chain). |
| @@ -62,7 +64,7 @@ class CriticalRequestChains extends Audit { | |||
| walk(chains, 0); | |||
|
|
|||
| return CriticalRequestChains.generateAuditResult({ | |||
| rawValue: chainCount.length <= this.meta.optimalValue, | |||
| rawValue: chainCount <= this.meta.optimalValue, | |||
There was a problem hiding this comment.
wow, this was just broken before. chainCount was not an array :\
Not sure if chains vs. depth is better, but if you undo line 53, I think it takes us back to a working audit.
|
@ebidel I changed the chain count to treat the initial navigation as special. Now, the walk method is only called when the initial navigation is known to have child, otherwise the chain count is left as 0. This does have the good sign that fewer changes are needed that the origin PR. |
| @@ -59,10 +59,16 @@ class CriticalRequestChains extends Audit { | |||
| }, ''); | |||
| } | |||
|
|
|||
| walk(chains, 0); | |||
| // Account for initiali navigation | |||
| const initialNavigationKey = Object.keys(chains)[0]; | ||
| const initialNavChildren = chains[initialNavigationKey].children; | ||
| if (initialNavChildren && | ||
| Object.keys(initialNavChildren).length > 0) { |
There was a problem hiding this comment.
indentation is off, but I think this can fit on one line.
That feels right to me and less confusing, but @brendankenny @paulirish might have insights that I don't...on why the initial initial page load was included in the chain. What I like about it not being included in the chain is that developers can actually pass this test now :) |
|
Yeah this is exactly what it shoulda been. Including the initial navigation as root never made sense. thanks mucho! |
|
Thanks for fixing this ridiculousness, @gauntface Really the intention was not to have a pass/fail at all, just to give you insight into what might be going wrong with your page load. Making a request is not a sin, obviously, and even multiple dependent requests may be necessary and depend on tradeoffs the dev is making in other areas. The point is to inform to make sure it's an intentional tradeoff, not an accidental one :) Giving a pass/fail to these is more an artifact of the report and scoring system, which will have to be fixed as we add more perf diagnostics. |
Ayup! 👍 |
* Make tests pass for lighthouse cli * Fixing up the tests to pass and return depth instead of chain count * Fixing up request chain to not account for initial navigation
Fixes #1270
This adds an example test that passes the audit (previously I think the code was fundamentally never going to pass).