jakesgordon / javascript-state-machine

A javascript finite state machine library
MIT License
8.69k stars 964 forks source link

onInvalidTransition 'to' argument is undefined #121

Open bjornhanson opened 7 years ago

bjornhanson commented 7 years ago

It seems the to argument passed to onInvalidTransition is always undefined, unless I'm not understanding things correctly. For example, if I create the following state machine:

var fsm = new StateMachine({
  init: 'A',

  transitions: [
    {name: 'one',   from: 'A', to: 'B'},
    {name: 'two',   from: 'B', to: 'A'},
    {name: 'three', from: 'B', to: 'C'}
  ],

  methods: {
    onInvalidTransition: function(transition, from, to) {
      console.log(arguments);
    }
  }
});

and then try to fire the three transition (while in state A), I would expect onInvalidTransition to be called with C as the value of to, but it's undefined.

> fsm.three();

(3) ["three", "A", undefined, callee: (...), Symbol(Symbol.iterator): function]

I didn't thoroughly examine the code, but it looks like the result of this.seek might only return a state to pass in to transit as to if it's a valid state to transition to:

  fire: function(transition, args) {
    return this.transit(transition, this.state, this.seek(transition, args), args);
  },

  transit: function(transition, from, to, args) {

    var lifecycle = this.config.lifecycle,
        changed   = this.config.options.observeUnchangedState || (from !== to);

    if (!to)
      return this.context.onInvalidTransition(transition, from, to);

    ...
  }
adami commented 7 years ago

why would you expect the "to" state to be anything? it's an invalid transition, meaning the state machine doesn't know where to transition to...

just as an example, if you also had another trasition from C to D that also happens on event 'three', which would you expect to get as "to"?

bjornhanson commented 7 years ago

You're right. Sorry, I was thinking about this all wrong. What I was trying to do is handle the error and allow transitions to the current state without triggering an event, but I don't think I can do that with just #onInvalidTransition.

It would be nice to avoid having to add a check using #can wherever I may want to fire a transition to the current state. For example, if I'm tracking network connection to the back-end with a state machine and I want some things to be able to fire fsm.connect() on an event, it might be nice to just ignore those transitions if we're already in that state instead of having to write extra code every time to check if it's a valid transition.

adami commented 7 years ago

I'm not sure i'm following what you're saying.

I think checking if the state machine can accept a transition before you fire it is the wrong way to go. Your development and testing process should catch all possible transitions and your code should handle them.

Yes, because it's the real world, there might always be flows you didn't consider, and that's what the onInvalidTrasition is for. checking if the state can accept the transition is a replacement for that, but a replacement causing your code to work hard for every transition.

In the specific case you're describing, you can just add a transition that stays on this state, i.e: {name: 'connect', from: 'A', to: 'A'}

note that in this case, when staying in the same state, some of the lifecycle methods are not being called:

            this.observersForEvent(lifecycle.onBefore.transition),
            this.observersForEvent(lifecycle.onBefore[transition]),
  changed ? this.observersForEvent(lifecycle.onLeave.state) : UNOBSERVED,
  changed ? this.observersForEvent(lifecycle.onLeave[from]) : UNOBSERVED,
            this.observersForEvent(lifecycle.on.transition),
  changed ? [ 'doTransit', [ this ] ]                       : UNOBSERVED,
  changed ? this.observersForEvent(lifecycle.onEnter.state) : UNOBSERVED,
  changed ? this.observersForEvent(lifecycle.onEnter[to])   : UNOBSERVED,
  changed ? this.observersForEvent(lifecycle.on[to])        : UNOBSERVED,
            this.observersForEvent(lifecycle.onAfter.transition),
            this.observersForEvent(lifecycle.onAfter[transition]),
            this.observersForEvent(lifecycle.on[transition])
bjornhanson commented 7 years ago

Yeah, I know what you're saying. It doesn't seem like state transitions should fire more than once. I don't think your example of adding a transition to stay on the same state would work. There's at least once case so far where I would want to fire a transition that I care about the first time (and get lifecycle events for), and then potentially ignore any after that. I may just have to add my own flags in my code to track the state and not rely on the state machine alone. There are events that occur that are out of my application's control (like messages coming in via websockets) where I would want to react to the first one and change state, but not every one after that. Right now I'm just evaluating if/where state machines may help clean up the code in our (already existing) application.