Process Managers: mutate state before handling event

[Cantido]

The handle-first behavior is awkward to work with, and seems confusing to others (see #176).
This change flips around the two calls, so a process manager's state is mutated before the event is handled.

I made sure not to ack the current event or serialize the new state until we're sure the handler function
has succeeded and commands have been dispatched.
That way, if the handler blows up, we aren't storing a corrupt state and can retry the apply & handle with no worries.

Fixes #176 .

[dwmcc]

Voicing agreement with @Cantido that having to handle first seems counter-intuitive to me. This change would be especially helpful for cases when the updated PM state is relied upon for the commands that are dispatched.

[slashdotdash]

This is a breaking change which would affect any existing process managers. Which is why I haven't merged the PR. I think It would need to be made opt-in with the default behaviour as is currently implemented (handle before apply).

[Cantido]

That's understandable. Is there any way in particular that you would like that implemented? Probably as an opt passed to use Commanded.ProcessManagers.ProcessManager, right?

[bitwalker]

I definitely found it unintuitive at first, but I think there is a good rationale for why it should be that way by default. First, I'd expect that you only want the process manager state to be changed once the event is handled successfully, as properly handling an event or an error is largely dependent on the last "committed" state. If you update the state first, then you've lost some context. That's not to say there aren't cases where updating the state first might be desirable, just that I think it is less frequent than the current approach. Making it optional shouldn't be for backwards compatibility, but rather to treat it like a new feature or an enhancement of an existing feature I suppose.

I'm not sure what I'd call the option though, which got me wondering if that's because it's so specific, or because two concepts are being conflated as a single "process manager" concept. When you update the state in response to handling the event, you are pretty clearly modeling a state machine, i.e. the transition between states is dependent on how the event is handled in the context of the starting state. The state of the process always reflects reality more or less. On the other hand, I think it could be said that when you handle an event in response to a state change, you aren't so much modeling a state machine, as you are modeling an orchestrator, where the state represents the "desired" state of the process being modeled, and the handler is responsible for acting to make that desired state the actual state. I may be missing other use cases for which the latter is used, but that's at least what comes to mind when I think about it. If that's the case, then it may make more sense to break these strategies up into distinct modules which are focused on their relative strengths/use cases. If the distinction is relatively minimal in terms of difference in the underlying implementation, then you might be able to just specify a "type" for the process manager as an option, as suggested by @Cantido.

Anyway, food for thought, but wanted to give my two cents. My initial reaction was that I wanted this too, but then I realized that most of the things I'm modeling are state machines, so I actually don't want the state updated first. I could imagine scenarios where I might have a mix of both though, so I'd be interested to see this move forward in some capacity.

[AHBruns]

Is there a reason to not just combine the callbacks? For example, get rid of apply/2 and have handle/2 be:

@callback handle(process_manager, domain_event) ::
  command |
  [command] |
  {process_manager, command} |
  {process_manager, [command]} |
  {:error, term}

[jvantuyl]

@AHBruns Generally one would expect apply/2 to not cause side-effects. Mixing them gives this up.