-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core(network-requests): add entity classification #15105
core(network-requests): add entity classification #15105
Conversation
isFirstParty
&& isThirdParty
to network-requests
auditisFirstParty
&& isThirdParty
to network-requests
audit
isFirstParty
&& isThirdParty
to network-requests
auditisFirstParty
& isThirdParty
to audit
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.
Should
isFirstParty
and/orisThirdParty
be added to the audit table output?
No, it's fine. network-requests
is a hidden audit so it would just be a few extra bytes with no benefit.
How do I update golden LHR?
yarn update:sample-json
core/audits/network-requests.js
Outdated
const mainResource = await MainResource.request({devtoolsLog, URL: artifacts.URL}, context); | ||
if (artifacts.GatherContext.gatherMode === "navigation") { | ||
const mainResource = await MainResource.request( | ||
{ devtoolsLog, URL: artifacts.URL }, |
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.
this and the change above look like they may be default prettier formatting or something?
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.
yep. it was. do y'all have formatter settings that can be applied to VSCode? (I could add to the contributing guide if there is)
isFirstParty
& isThirdParty
to auditentity
to audit
@brendankenny PTAL when you get the chance. Is there anything I've missed here? |
core/audits/network-requests.js
Outdated
@@ -77,6 +83,7 @@ class NetworkRequests extends Audit { | |||
priority: record.priority, | |||
isLinkPreload, | |||
experimentalFromMainFrame, | |||
entityName, |
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.
We could name this as entity
to match the pattern of audit-side classification. Third-party summary audit is a good example.
When the entity
property is provided from an audit, the report rendering side skips classification and relies on the provided one. See report-side classification yielding to audit provided entity
.
In this case the audit isn't rendered, but would be good to keep the same convention.
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.
Thanks for review @alexnj. PTAL 🙏
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.
LGTM. Left one comment.
@adamraine I think this is ready to go 👍 |
entity
to audit
Summary
At present it's difficult to collect which requests are first and third party in LH. As of this PR,
entity
will be present in thenetwork-requests
audit output.Related Issues/PRs
requestCount
tothird-party-summary
subitem attributes #15093