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): Deprecate 'performance-budget' audit / add 'resource-budget' audit #9907

Closed
wants to merge 2 commits into from

Conversation

khempenius
Copy link
Collaborator

@khempenius khempenius commented Oct 31, 2019

  • Deprecates performance-budget audit
    • Will not appear in the HTML report, but will remain in the JSON
    • Warning and log message inform user that they should switch to using resource-budget
  • Adds resource-budget audit
    • This audit is identical to the existing performance-budget audit.
    • Appears in HTML report & JSON.

Motivation
As part of supporting timing budgets, the functionality of the performance-budget audit is being renamed to resource-budget. (And timing-budget is being added as a separate audit.) However, for the time being, performance-budget is being left in codebase for sake of smooth deprecation.

Related Issues/PRs
#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.

sheesh has our checked-in boilerplate really ballooned

image

a file and id rename turns into 900 changes 😆

@khempenius khempenius changed the title core(lightwallet): Rename 'performance-budget' audit to 'resource-budget' core(lightwallet): Deprecate 'performance-budget' audit / add 'resource-budget' audit Oct 31, 2019
@patrickhulce
Copy link
Collaborator

performance-budget is being left in codebase for sake of smooth deprecation

Oh sorry I just meant to bring it up for discussion I'm not 100% convinced we should duplicate it yet :D

Anyone else have thoughts here? If it weren't treated specially as part of the report UI, I'd be all for just nuking it and doing the rename but the specialness of this audit makes it worth consideration at least IMO :)

@patrickhulce
Copy link
Collaborator

Discussing this in LH sync today my understanding is that we'd prefer to just leave the ID as-is performance-budget and live with the minor inconsistency. We still keep the label/string changes here to reflect that they're only addressing resources. We can announce that we intend to rename this to resource-budget in a future breaking change and can expose this also under resource-budget in a future non-breaking version, but we wanted to leave more warning time on this one.

@khempenius how does that sound to you?

If anyone had a different understanding from the meeting please speak up :)

@khempenius khempenius closed this Nov 5, 2019
@khempenius khempenius deleted the budget_rename branch November 5, 2019 19:20
@connorjclark
Copy link
Collaborator

I'll voice an opinion no one else shared: If we are going to make this breaking change, and a programmatic deprecation warning is not possible (as Paul mentioned), I think it best to do it as early as possible to minimize total pain (huh, I guess I'm a utilitarian)

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