Skip to content
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(lightwallet): add timing-budget audit #9901

Merged
merged 6 commits into from
Nov 5, 2019
Merged

core(lightwallet): add timing-budget audit #9901

merged 6 commits into from
Nov 5, 2019

Conversation

khempenius
Copy link
Collaborator

Adds audit only; does not add it to default config or add renderer code. That will be done in a later PR.

This audit returns details in the following format:

{
  metric: 'first-contentful-paint',
  label: 'First Contentful Paint',
  measurement: 1256.3333,
  overBudget: 562.53444
}

Background info on timing budgets implementation:
The performance-budget audit is becoming resource-budget audit; timing budgets are being added as a a separate timing-budget audit. Both audits are part of the budgets group.

Issue: #8917

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lookin' good!

Probably should have brought this up earlier, but have we given thought to what's going to happen with our first non-timing metrics like CLS when they land? Will they be covered by budgets.timing and this audit? Will we need a new audit? Will we go through another audit id rejiggering? May not need to decide right now, but it'd be nice to avoid a breaking budgets change every major if we can plan ahead :)

lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/timing-budget-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/timing-budget-test.js Outdated Show resolved Hide resolved
@khempenius
Copy link
Collaborator Author

@patrickhulce

I think something like CLS should be covered by a separate section in budget.json. The exact name would need more thought, but it could be something like budget.layout or budget.metrics, etc. & the section would have its own corresponding audit. This would not entail any breaking changes or audit id renames.

I also think you might also be able to make the argument that something like CLS should be enforced via lighthouse-ci rather than budget.json.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! excited for the surfacing phase :D

@@ -139,7 +137,7 @@ class ResourceBudget extends Audit {
{key: 'requestCount', itemType: 'numeric', text: str_(i18n.UIStrings.columnRequests)},
{key: 'size', itemType: 'bytes', text: str_(i18n.UIStrings.columnTransferSize)},
{key: 'countOverBudget', itemType: 'text', text: ''},
{key: 'sizeOverBudget', itemType: 'bytes', text: str_(UIStrings.columnOverBudget)},
{key: 'sizeOverBudget', itemType: 'bytes', text: str_(i18n.UIStrings.columnOverBudget)},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@exterkamp should we be keeping in mind any particular timeline for tc roundrip of strings that we need to release in time? I guess we're only targeting an alpha for CDS but having all the metrics disappear for example would be bummer

lighthouse-core/lib/i18n/i18n.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/timing-budget.js Outdated Show resolved Hide resolved
types/artifacts.d.ts Show resolved Hide resolved
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
@vercel vercel bot temporarily deployed to staging November 4, 2019 19:45 Inactive
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, thanks for pulling in that TimingSummary fix, too!

LGTM

@brendankenny brendankenny merged commit 5f2b73e into GoogleChrome:master Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants