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 performance-budget audit #8539

Merged
merged 11 commits into from
May 4, 2019
Merged

core(lightwallet): add performance-budget audit #8539

merged 11 commits into from
May 4, 2019

Conversation

khempenius
Copy link
Collaborator

Summary
This audit implements the budgets table.

Note: This branch also contains the code from #8522 and #8427, which are upstream dependencies.
The code changes unique to this branch are in resource-budgets.js and resource-budgets-test.js.

With budgets.json:
Screen Shot 2019-04-23 at 2 35 19 AM

Without budgets.json:
Screen Shot 2019-04-23 at 2 26 29 AM

Related Issues/PRs

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.

Nothing else jumps out to me as major structural changes here, so I'll get back on the other two :)

So swift with these PRs @khempenius nice work! 💯

lighthouse-core/audits/resource-budgets.js Outdated Show resolved Hide resolved
lighthouse-core/config/default-config.js Outdated Show resolved Hide resolved
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.

some early feedback, though obviously will be changing as it accounts for changes in the computed artifact and the other audit PR

lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/config/budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
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.

looking good, just some style feedback for now.

Agreed with @paulirish on the notApplicable fallback if there's no budget, since it looks like (until we have a default budget) the defaultTable will just be a repeat of the resource-summary table

lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-budget.js Outdated Show resolved Hide resolved
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.

nice!

I think it still needs a yarn update:sample-json

lighthouse-core/audits/performance-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/performance-budget.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/performance-budget-test.js Outdated Show resolved Hide resolved
@brendankenny
Copy link
Member

I left this comment on like three other PRs as well, but the changes to proto/sample_v2_round_trip.json after #8818 might be so severe that you might need to merge from master (to pick up the proto test changes), and nuke the test json with git checkout master -- proto/sample_v2_round_trip.json and run update:sample-json again. Hard to tell what happened to all these PRs...

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.

LGTM! (with a few last trivial things)

lighthouse-core/audits/performance-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/performance-budget.js Outdated Show resolved Hide resolved
lighthouse-core/audits/performance-budget.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/performance-budget-test.js Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to staging May 3, 2019 19:13 Inactive
@brendankenny brendankenny changed the title feat(lightwallet): Add 'resource budgets' audit core(lightwallet): add performance-budget audit May 3, 2019
@brendankenny
Copy link
Member

with the fix, over to you @paulirish

@khempenius
Copy link
Collaborator Author

Updated.

@paulirish paulirish merged commit 5aec063 into GoogleChrome:master May 4, 2019
@brendankenny
Copy link
Member

📏🐢⏱🐎⚖️🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants