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

allow for "budgets" as a deprecation handler #64

Open
fivetanley opened this issue Aug 12, 2019 · 4 comments
Open

allow for "budgets" as a deprecation handler #64

fivetanley opened this issue Aug 12, 2019 · 4 comments

Comments

@fivetanley
Copy link

fivetanley commented Aug 12, 2019

Context

Currently, ember-cli-deprecation-workflow supports 3 handlers for deprecations:

silence Keeps this deprecation from spewing all over the console
log Normal deprecation behavior runs for this deprecation and messages are logged to the console
throw The error is thrown instead of allowing the deprecated behavior to run. WARNING: APPLICATION MAY GO 💥

While these work for their intended purposes really well, they don't necessarily help cases where you might not be able to address a deprecation right away, but a team wants to prevent the deprecation count from growing from things like copy-pasting code that will cause new deprecations to pile up.

Proposal

Users of this addon will be able to specify a budget option to the handler option for each deprecation. If the deprecation is triggered more than the allowed budget in certain scenarios (e.g. a test suite run where conditions are more controlled), then the handler will throw. This would allow a team to keep existing code that causes deprecations but fail builds in CI for any new code that increases deprecation use.

For example, take the following config/deprecation-workflow.js:

// config/deprecation-workflow.js
window.deprecationWorkflow = window.deprecationWorkflow || {};
window.deprecationWorkflow.config = {
  workflow: [
    { matchId: 'ember-string-utils.fmt', handler: { budget: 10 } },
  ]
};

if a new pull request was opened with the following code:

Ember.String.format('some string %s', 'hi')

This would increase the number of deprecations invoked to 11, violating the budget, causing ember-cli-deprecation-workflow to throw, failing the build.

We would likely also need some facility to make copy/pasting the existing deprecation count (to set the initial budget) to config/deprecation-workflow.js from the browser easier as well.

I'd be happy to PR this, just wanted to get thoughts on the idea before going through the effort of writing code.

@patocallaghan
Copy link

@fivetanley Just to clarify, does budget count the number of unique codepaths a deprecation is fired from or is it the total number of times the deprecation warning is called no matter where it's called from?

For example, adding a new test which exercises existing deprecated code won't cause you to blow your budget?

@fivetanley
Copy link
Author

@patocallaghan the total number of times the deprecation warning is called no matter where it's called from. trying to track where something gets triggered from sounds expensive/ hard to know, but if there's a good way to track that / figure it out that might be a more useful thing

@bantic
Copy link

bantic commented Aug 29, 2019

I think this is an excellent idea, but I'd like to suggest that an alternative property name be used, as budget is somewhat overloaded as a term (it makes me think first of performance budgets) and doesn't make it clear what will happen (throw|log) when the budget is met.

I'd suggest something like: throwAfter, throwAfterOccurrences, throwAfterMinimum or similar.

Additionally, it seems like it might also be useful to make this configurable so that you can set a budget and then choose whether additional deprecations log or throw. In that case, the term 'budget' would make sense again because contextual options could be used that clarify its meaning. Perhaps like this?

window.deprecationWorkflow.config = {
  workflow: [
    { matchId: 'ember-string-utils.fmt',
      // Will log after the 10th occurrence
      handler: 'budget', options: { count: 10, behavior: 'log' }
    },
  ]
};

@mixonic
Copy link
Member

mixonic commented Aug 30, 2019

Oh I think I like @bantic's last suggestion at API there, especially since it moves options off of the handler property.

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

No branches or pull requests

4 participants