-
Notifications
You must be signed in to change notification settings - Fork 9.6k
core(third-party-summary): add blocking time impact #9486
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
Changes from 4 commits
d70939c
e434fd4
bbd5328
f46e441
9db995b
782ad13
d55a822
676b9b0
9b36046
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,25 +14,30 @@ const NetworkRecords = require('../computed/network-records.js'); | |||||||||||
| const MainThreadTasks = require('../computed/main-thread-tasks.js'); | ||||||||||||
|
|
||||||||||||
| const UIStrings = { | ||||||||||||
| /** Title of a Lighthouse audit that identifies the code on the page that the user doesn't control. This is shown in a list of audits that Lighthouse generates. */ | ||||||||||||
| title: 'Third-Party Usage', | ||||||||||||
| /** Title of a diagnostic audit that provides detail on the code on the page that the user doesn't control (referred to as "third-party code"). This descriptive title is shown to users when the amount is acceptable and no user action is required. */ | ||||||||||||
| title: 'Third-Party usage', | ||||||||||||
| /** Title of a diagnostic audit that provides detail on the code on the page that the user doesn't control (referred to as "third-party code"). This imperative title is shown to users when there is a significant amount of third-party execution time that should be reduced. */ | ||||||||||||
patrickhulce marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||
| failureTitle: 'Reduce impact of third-parties', | ||||||||||||
patrickhulce marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||
| /** Description of a Lighthouse audit that identifies the code on the page that the user doesn't control. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ | ||||||||||||
| description: 'Third-party code can significantly impact load performance. ' + | ||||||||||||
| 'Limit the number of redundant third-party providers and try to load third-party code after ' + | ||||||||||||
| 'your page has primarily finished loading. [Learn more](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/loading-third-party-javascript/).', | ||||||||||||
| /** Label for a table column that displays the name of a third-party provider that potentially links to their website. */ | ||||||||||||
| columnThirdParty: 'Third-Party', | ||||||||||||
| /** Label for a table column that displays how much time each row spent executing on the main thread, entries will be the number of milliseconds spent. */ | ||||||||||||
| columnMainThreadTime: 'Main Thread Time', | ||||||||||||
| /** Label for a table column that displays how much time each row spent blocking other work on the main thread, entries will be the number of milliseconds spent. */ | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there might have been an issue with describing the "main thread" to the tc... @exterkamp? Or was it for this audit and it's already been clarified :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem was with "thread" being used, in regards to the description in I think this is fine here. Maybe add something about threads. But I think that is unnecessary. This LGTM. |
||||||||||||
| columnBlockingTime: 'Main-Thread Blocking Time', | ||||||||||||
| /** Summary text for the result of a Lighthouse audit that identifies the code on the page that the user doesn't control. This text summarizes the number of distinct entities that were found on the page. */ | ||||||||||||
patrickhulce marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||
| displayValue: `{itemCount, plural, | ||||||||||||
| =1 {1 Third-Party Found} | ||||||||||||
| other {# Third-Parties Found} | ||||||||||||
| }`, | ||||||||||||
| =1 {1 third-party} | ||||||||||||
| other {# third-parties} | ||||||||||||
| } blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`, | ||||||||||||
|
||||||||||||
| } blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`, | |
| `{itemCount, plural, | |
| =1 {1 third-party blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`} | |
| =other {# third-parties blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`} | |
| }`, |
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 would also expand on "third-party" with something like 1 third-party script blocked...
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 totally forgot about this whole fiasco, honestly I don't think the number of third parties needs to be communicated here if it causes trouble
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.
maybe put some version of the comment in the PR description
It shows up as failing if the impact to TBT is >250ms (which would push you out of a ~90 score range on TBT)
in here to give motivation to the particular number?
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.
yeah sg
| // A page passes when all third-party code blocks for less than 250 ms. | |
| // A page fails this audit if the main-thread impact is >250ms (which would push you out of a ~90 score range on TBT). |
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.
is this a TODO or worth reexamining at some point?
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.
IMO not a TODO ever worth fixing since it should be rare that third party code actually blocks FCP and if it does I'm fine pointing the finger at it anyway. Our slight decoupling of the audit from TBT in external descriptions also makes this less of an issue.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,8 @@ describe('Third party summary', () => { | |
|
|
||
| const results = await ThirdPartySummary.audit(artifacts, {computedCache: new Map()}); | ||
|
|
||
| expect(results.displayValue).toBeDisplayString('2 Third-Parties Found'); | ||
| expect(results.score).toBe(1); | ||
| expect(results.displayValue).toBeDisplayString('Increased Total Blocking Time by 20 ms'); | ||
| expect(results.details.items).toEqual([ | ||
| { | ||
| entity: { | ||
|
|
@@ -31,6 +32,7 @@ describe('Third party summary', () => { | |
| url: 'https://marketingplatform.google.com/about/tag-manager/', | ||
| }, | ||
| mainThreadTime: 104.70300000000002, | ||
| blockingTime: 18.186999999999998, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just interesting to see the impact here drop (104ms -> 18ms)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I'm a bit disappointed to see the very busy third parties disappear, but if it's an incentive to script responsibly than I guess it's still a very positive outcome so I'm :) My biggest concern is that no one is going to understand what "main-thread blocking time" actually is and they'll assume it's the time spent which would be sad. FWIW this is also not as drastic in the lantern case, it goes from 419 -> 250. While still a big drop, the number is still noticeable. |
||
| transferSize: 30827, | ||
| }, | ||
| { | ||
|
|
@@ -40,6 +42,7 @@ describe('Third party summary', () => { | |
| url: 'https://www.google.com/analytics/analytics/', | ||
| }, | ||
| mainThreadTime: 87.576, | ||
| blockingTime: 0, | ||
| transferSize: 20913, | ||
| }, | ||
| ]); | ||
|
|
@@ -54,9 +57,12 @@ describe('Third party summary', () => { | |
| const settings = {throttlingMethod: 'simulate', throttling: {cpuSlowdownMultiplier: 4}}; | ||
| const results = await ThirdPartySummary.audit(artifacts, {computedCache: new Map(), settings}); | ||
|
|
||
| expect(results.score).toBe(0); | ||
| expect(results.details.items).toHaveLength(2); | ||
| expect(Math.round(results.details.items[0].mainThreadTime)).toEqual(419); | ||
| expect(Math.round(results.details.items[0].blockingTime)).toEqual(250); | ||
| expect(Math.round(results.details.items[1].mainThreadTime)).toEqual(350); | ||
| expect(Math.round(results.details.items[1].blockingTime)).toEqual(157); | ||
| }); | ||
|
|
||
| it('be not applicable when no third parties are present', async () => { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.