-
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
Gatherer for critical request chains #300
Conversation
const log = require('../lib/log.js'); | ||
|
||
const flatten = arr => arr.reduce((a, b) => a.concat(b), []); | ||
const contains = (arr, elm) => arr.indexOf(elm) > -1; |
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.
arr.includes(elm)
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.
doesn't work in node :(
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.
It's in 6.1.0, it would appear. I guess at this point since most people are bypassing 5 for 6 we could specify it as a minimum?
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.
It works on 6.0 too. Once we drop support for node 5.0 we can change this.
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'm +1 on supporting 5.
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.
just FYI for the future: v8 syntax changes only ever happen in the major node version bumps.
As much as I want destructuring and rest parameters, I'd also like to hold off on requiring 6 for a bit longer.
One minor nit / query but LGTM otherwise. Terrific work as usual. |
|
||
class CriticalNetworkChains extends Gather { | ||
|
||
get criticalPriorities() { |
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.
Let's add a comment with link to https://docs.google.com/document/u/1/d/1bCDuq9H1ih9iNjgzyAL0gpwNFiEP4TZS-YLRp_RuMlc/edit to explain this.
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.
Done
@deepanjanroy a few sites that should have decent chains: imore.com |
Edit (may 11, paulirish): This is PR is mostly superseded by #310, but it's being retained for the moment to confirm that we have matching data on both sides.
Progress on #292
We're using the clovis network dependency graph. The new gatherer is hidden behind
--use-net-dep-graph
flag, so the extension still builds and runs fine.Example execution:
lighthouse http://theverge.com --use-net-dep-graph
. Since we do not yet have an auditor or anything to consume the gather data, chains with at least two requests are logged to console so you can see the results.The gatherer returns all critical requests chains, and all the requests in a critical request chain has to be critical (as opposed to only the beginning and end of the chain.) As a quick example, this request pattern:
will return
[A, B, C]
and[A, D]
as critical chains.