Discussion: rethinking callbacks #803

See prior discussion on this in #524

We've begun to stretch the limits of our existing callback system (dataCallback, shouldSendCallback, breadcrumbCallback). Each of these can be specified during config, or via corresponding setX methods (e.g. setDataCallback).

Example:

Raven.setDataCallback(function (data, originalCallback) {
  data = originalCallback && originalCallback.apply(this, arguments);
  // do things to `data`
  return data;
});

Downsides with this system:

• Cannot easily introduce new parameters, e.g. error (the original error object that triggered the callback) (see Provide a new hook for harvesting "extra" data from an Error object #483)
  • This could be done today, but would mean having a signature of (data, originalCallback, originalError), which is burdensome when invoking originalCallback.
• Relies on user properly invoking originalCallback in order to preserve the callback chain
  • For example, the Angular plugin uses a dataCallback to parse Angular error messages. If the user overrides the callback via setDataCallback, they must invoke originalCallback properly or the Angular message parsing is skipped.
• Need an addX method for each callback type (adds to code/API bloat)
• Doesn't work with asynchronous code – must return value synchronously (see Switched to using callback for determining if error should be sent #314)

Pros:

• Flexible. A future-set callback can intentionally prevent invocation of a previously set callback. Or it can intentionally invoke that callback before, or after, its own invocation.
  • Not sure how useful this is in practice, but it avoids the "before" and "after" problem of some event systems.

There was a previous discussion on this topic in #524, where I proposed an event-based system (similar to Backbone.Events or jQuery) (implementation in #521), but it didn't really have much steam behind it, and felt too different from what we're doing.

Goals of a new system

• Callbacks can both mutate and cancel (e.g. merge concepts like shouldSendCallback AND dataCallback into one – this is already done in breadcrumbCallback)
• More futureproof / flexible (can introduce new parameters in future)
• Easy to use (can't easily clobber preexisting callbacks)
• Replaces undocumented DOM events (ravenSuccess, ravenError)
• As backwards-compatible as possible (i.e. at very least, old methods should still work with same signature)

Proposal 1: Middleware-inspired

Inspired by Express, Rack, etc:

/**
 * data {Object} outbound data payload
 * err {Error} error object that generated data
 * next {Function} invoke to progress to next handler
 */
Raven.on('data', function (data, err, next) {
  // do stuff
  next(false); // stop propagation
  next(data); // pass new data object, continue with propagation
  next(); // data unchanged, continue with propagation

  setTimeout(next.bind(this, false), 1000); // async example
});

Raven.on('error', function (data, req, res, next) {});
Raven.on('success', function (data, req, res, next) {});
Raven.on('breadcrumb', function (breadcrumb, evt, next) {});

Taking a cue from Mocha, we could inspect each callback's arity (via Function.length) and decide whether it is "async" or not. This means users could drop the next param, and do:

Raven.on('data', function (data, err) {
  // do stuff
  return false; // stop propagation
  return data;  // pass new data object, continue with propagation
  return; // data unchanged, continue with propagation
});

Raven.on('error', function (data, req, res) {});
Raven.on('success', function (data, req, res) {});
Raven.on('breadcrumb', function (breadcrumb, evt) {});

Pros

• asynchronous
• low footprint (only one method to add callback)

Cons

• cannot change order of middleware invocation (always FIFO or FILO)
• cannot easily add new parameters

Questions

• is there a need for an off method to remove previously set handlers?

Comments welcome.

cc @LewisJEllis @blittle @lbntly @keithhalterman