Add generic error handler that can potentially be overriden by librar…

[yarandoo]

This PR is a suggestion to deal with #14. With it users of the library can provide their own error handling logic by overriding the error function.

In our case this PR allowed us to define a custom handler when you throw Exceptions like "Invalid event in current state", and then we can translate that Exception to one of our own. Thus we define this in a central place instead of needing to catch every time we call any event, and do the proper translation.

What do you think?

[coveralls]

Coverage remained the same at 97.714% when pulling 470d584 on yarandoo:generic-error-handler into 143f376 on vstirbu:master.

[vstirbu]

Thanks for bringing this out! The error handling is a delicate topic in the context of promises, and especially so when having promise chains.

Can you provide a bit more context in how you use it? The PR is just aggregating the error throwing into a single place, which is certainly of value, but are you doing something more than that?

this.error is still internal to the library. Do you intend to completely replace the error throwing function with a custom function that is more appropriate for handling the error is your application domain (the PR name implies that)?

[yarandoo]

Yes, when initializing the state machine I am providing my own error
handling function as part of the configuration options.

I.e:

var myErrorHandler = function (msg, options) {
    throw new MyApplicationCustomError('Error in state-machine', msg);
}
var fsm = StateMachine({
    initial: 'here',
    events: [
        { name: 'jump', from: 'here', to: 'there' }
    ],
    error: myErrorHandler,
});

[vstirbu]

The PR in the current state extends the configuration object with a new property error.

What about providing the error as part of the callbacks:

var fsm = StateMachine({
  initial: 'red',
  events: [
    { name: 'red', from: 'green', to: 'red' }
  ],
  callbacks: {
    error: function(msg, option) {
      throw new Error('my error');
    }
  }
});

in which case the above

throw new this.factory.FsmError(msg, option);

becomes:

if (this.callbacks.error) {
  return this.callbacks.error(msg, option);
} else {
  throw new this.factory.FsmError(msg, option);
}

[vstirbu]

I like the idea of being able to replace the default error handling but I'm not sure this way is the most appropriate one. Right now these are the ways to customise this behaviour:

Configure the StateMachine library

StateMachine.errorHandler = (msg, option) => {
   //my handler
}

advantages:

• similar with configuring Promise library or the callbackPrefix, parameters that determine how the object created by the state machine works.

disadvantages:

• keep track of different error handlers if need for multiple such cases within the same application

Extend the configuration object

var fsm = StateMachine({
    initial: 'here',
    events: [
        { name: 'jump', from: 'here', to: 'there' }
    ],
    error: myErrorHandler,
});

advantages:

disadvantages:

• extends the configuration object

Provide s callback

var fsm = StateMachine({
  initial: 'red',
  events: [
    { name: 'red', from: 'green', to: 'red' }
  ],
  callbacks: {
    error: function(msg, option) {
      throw new Error('my error');
    }
  }
});

advantages:

• fsm instance specific callback

disadvantages:

• possible collisions with a state machine having error event and callbacksPrefix = '' (empty string)

[yarandoo]

Extend the configuration object

The idea of having the custom error handler in the configurationn follows the same style as jakesgordon fsm:

https://github.com/jakesgordon/javascript-state-machine#handling-failures

I dont see why providing this in the config would be a disadvantage? As in my opinion this is a actually a configuration property.

The other options are worth evaluating too, is good that you brought them up.

Providing the error function as a callback:

The error handler is actually not a callback related to the events per se. Wouldn't we be overloading the definition of callbacks here? Also as you said it could have some collisions...

Configure the StateMachine library

This could be an option, although it would be nice if each instance can define its own error handler? I would say that this option is not exclusive of the other two. We can have a general error handler in case we want to define one for all instances and a instance specific handler.?

[vstirbu]

Extending the configuration object is the option that encapsulates quite well the customisation of the euro handling to a specific fsm instance.

However, the PR leverages too well the current library implementation. It is worth mentioning that this is quite accidental as the current implementation is somewhere midway from the pre-stamp-it implementation to a desired state where we leverage fully composability features enabled by stamp-it (see #17 for wanted API features).

Let me digest a bit how to move this forward. I don't want to move too fast without careful consideration on impact on evolution.

[vstirbu]

Been thinking about a way forward that brings together error, Promise and callbacksPrefix. The last two are library configurations while the error is instance configuration. What about adding a new configuration property that can hold these three and future per instance configurations which are not strictly related to how the state machine works from application domain perspective:

StateMachine({
  initial: 'start',
  final: 'end',
  events: [
    { name: 'go', from: 'start', to: 'middle' },
    { name: 'further', from: 'middle', to: 'end' }
  ],
  callbacks: {

  },
  ops: {
    callbackPrefix,
    error,
    Promise
  }
});

In the example above the ops comes from operations, but any other name that describes this is fine.