ember-cli / ember-cli-deprecation-workflow

MIT License
165 stars 43 forks source link

allow for "budgets" as a deprecation handler #64

Open fivetanley opened 5 years ago

fivetanley commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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