Discussion: rethinking callbacks

[benvinegar]

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 #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 #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

[blittle]

As long as you are considering a breaking API change, would it be worth considering the way Redux does middleware life-cycle events? http://redux.js.org/docs/advanced/Middleware.html

[benvinegar]

Just updated with a proposal: "Middleware-inspired approach".

This approach solves a lot of problems, but I'm concerned that requiring developers to invoke next could make it error prone.

Just edited the proposal to allow users to specify a next-less callback, in which case we treat the callback as synchronous and just evaluate the return value. This would be similar to how Mocha behaves differently if your test function declares a done parameter.

[benvinegar]

Another thought:

Why not just have every callback accept some kind of params object, instead of declaring individual arguments? That way we can add as many "parameters" as we want, depending on the callback. That means the signatures change to:

Raven.on('success', function (params, next) { ... });

Where params is an object with contextual parameters. For example, for the data callback we have:

{
  payload: { ... } // outbound data object
  error: { ... } // error that created this payload
}

Whereas the success callback has:

{
  payload: { ... } // outbound data object
  request: { ... } // request object (if available)
  response: { ... } // response object (if available)
}

[blittle]

Looks good to me!

[kamilogorek]

Closing as discuss will be moved to v4 docs. Linked this issue there.