Transition in strict node does not report errors when event is not handled by StateNode

Obi-Dann commented 6 years ago

Bug (at least I think so)

There seems to be no way to determine if an event can be handled by the current state node. For example:

const { Machine } = require('xstate');

const machine = Machine({
    initial: 'idle',
    strict: true,
    states: {
        idle: {
            on: {
                START: 'processing'
            }
        },
        processing: {
            on: {
                DONE: 'idle'
            }
        },
    }
});

let state = machine.initialState;

function transition(event) {
    const originalValue = state.value;
    state = machine.transition(state, event);
    console.log(`State changed from '${originalValue}' to '${state.value}'`);
}

transition('DONE');

Expected result:

// => Error: Machine '(machine)' does not accept event 'DONE'
// or:
// => Error: State node 'idle' does not accept event 'DONE'

Actual result:

// Actual:
// => State changed from 'idle' to 'idle'

Link to reproduction:

https://codepen.io/anon/pen/qKXbEZ

Discussion

I believe it's a bug, or at least there should be a way to avoid transitions. There's handles function that I can potentially use with machine.getStateNode, eg:

const stateNode = machine.getStateNode(state);
const isSupported = stateNode.handles(event);

However, it might not be as easy to implement for nested machines and I believe it should be checked in transition when running in strict mode.

Am I missing something ?

davidkpiano commented 6 years ago

This seems related to a discussion we've been having in the Gitter (cc. @mogsie @mpolichette).

I think what we want to do is add a property to the State instance that answers the question:

Did this event result in a state transition?

With the results:

✅ state transitioned to a different state
✅ state transitioned to a different child state
✅ state explicitly transitioned to self (internal or external)
- Note: the state value doesn't change in this instance, but it still counts
❌ state did not transition; event was not handled by any state node
:x: state did not transition; event was handled but next state target is undefined or null

Initially I was thinking of adding a changed: true property to the State instance, but we can bikeshed that.

What I don't think we should do (at least by default) is throw an error if an event is unhandled. Happy to discuss this, though.

mogsie commented 6 years ago

I agree, except maybe for the last "state did not transition, event was handled but next state target is undefined or null" — To me it's an indication of the statechart explicitly understanding the event, but choosing not to do anything. I'll have to think about it a bit, maybe come up with some more use cases, I guess :-)

Edit: Oh, and I agree: We should definitely not throw an error if the event was ignored :-)

mogsie commented 6 years ago

To me, it makes sense that for the states

states: {
    idle: {
        on: {
            START: 'processing'
        }
    },
    processing: {
        on: {
            START: null, // or START: '' ?
            DONE: 'idle'
        }
    },
}

then machine.transition('idle', 'DONE') would return an object that indicated:

that the current state is (still) idle (current behaviour)
that none of the active states consumed the event (the new functionality)

Conersely, machine.transition('processing', 'START') would return an object that indicated:

that the current state is (still) processing (current behaviour)
that the event was consumed.

In my opinion, the fact that a state transition occurred or not is not worth exposing. If it's important, then you should add an action on the state entry, and react to that signal instead. Knowing if an event was consumed or not is the important part.

davidkpiano commented 5 years ago

the fact that a state transition occurred or not is not worth exposing

This is now under state.changed (boolean | undefined).

There seems to be no way to determine if an event can be handled by the current state node

This is now under state.nextEvents and can be used like:

const currentState = machine.transition(...);

// Does it handle the FOO event?
currentState.nextEvents.includes('FOO');
// => true | false

statelyai / xstate