-
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
Update: Use array of non-critical resource types #2191
Conversation
@@ -34,12 +34,13 @@ class CriticalRequestChains extends ComputedArtifact { | |||
*/ | |||
isCritical(request) { | |||
// XHRs are fetched at High priority, but we exclude them, as they are unlikely to be critical |
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.
the comments relate to the nonCriticalResourceTypes
so they shoudl probably be right on top of it.
WebInspector.resourceTypes.Image._category, | ||
WebInspector.resourceTypes.XHR._category | ||
]; | ||
if (nonCriticalResourceTypes.includes(resourceTypeCategory)) { |
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.
the request._resourceType &&
conflicts with this.
one solution: you can do an early return false
if _resourceType
is falsy.
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.
ah nevermind. at worst it'll be [string,string].includes(undefined)
which will just fail, but not throw.
no change necessary for 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.
thx man! lgtm
appreciate the assist on that one! |
@paulirish Follow up PR for #2085
Figured I would add it since I suggested and had time now