Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API revision: coupling action and condition in transitions (QoL) #1038

Closed
MrNovado opened this issue Mar 2, 2020 · 2 comments
Closed

API revision: coupling action and condition in transitions (QoL) #1038

MrNovado opened this issue Mar 2, 2020 · 2 comments

Comments

@MrNovado
Copy link

MrNovado commented Mar 2, 2020

Point

I think it would be beneficial to have a simplified version of transitional-action, which couple actions and conditions together:

// on: 
PLAY: [
  // first transition with a try to return an action (or a list) wins
  {
    // coupled action and condition:
    // (context, event) => action | false
    try: "try1",
  },
  {
    try: (context) => context?.x ? <send|sendParent|assign|...anyAction>() : false,
  },
  /*
  {
    trys: ["try1", "try2", "try3", "try4"]
    list of trys like that makes no sense?
    would it then make more sense to have try as a
    (context, event) => action | action[] | false
  }
  */
  {  
    try: (context) => contex?.y && context?.z 
        ? [send("Y", { y: context.y })
          ,sendParent(/* Z */)
          ]
        : false,
  },
],

So instead of declaring transitions like this:

{ target: string, cond: (context, event) => boolean, actions: string | action | action[] } 

I think it would be nice to have a more concise version like that:

{ target: string, try: (context, event) => action | action[] | false }

Reasoning

I've been trying to make my point in #1036, but I got lost in semantics and mixed the transitional-action with conditional-action #990.

Conditional actions are cool, and it makes a lot of sense for them to exist, but the underlying issue I want to solve here is unrelated to them.

The issue with the current way of making transitions is that actions and conditions are decoupled, making it awkward to use them when in order to check a condition you have to make the same'ish amount of work needed to make an action.

  • Like, for an actor in a game, in order to make a proper move, you'll have to find a desirable spot first and then check if it's free to move to, concluding the conditional check.
  • But since actions are decoupled from conditions, you will have to find a desirable spot again (which we already did, but the action has no clue about, and we couldn't buffer our findings anywhere) and then message a parent with the selected spot.

Additionally, conditional-guards could not be used to restrict the context of execution (with typescript) in any way, leaving the action completely clue-less about the current state, forcing it to recalculate and re-check everything anew.

So really, is what I'm asking here is maybe just a sugar-coating/optimization for an already existing feature. The addition of this feature should not produce any breaking changes. This is purely QoL (and might be completely optional?).

There's a workaround you could use, like putting the result of a condition-check in a buffer first, and then reason about it, but it will most likely require you to remodel your state machine (like shown below).

This is easier to understand with an example.

Example

From #1036:

I've been tinkering with the tic-tac-toe game for a while now:
https://github.com/MrNovado/xstate-playground/tree/master/src/feature/TicTacToe
https://mrnovado.github.io/xstate-playground/#/x-tictactoe

And one of my actors has this list of transitions:
https://github.com/MrNovado/xstate-playground/blob/master/src/feature/TicTacToe/Actor.declarative.ts#L162

PLAY: [
  {
    // 1. Win: If the player has two in a row,
    // they can place a third to get three in a row.
    actions: "win",
    cond: "checkMy2InARow",
  },
  {
    // 2. Block: If the opponent has two in a row,
    // the player must play the third themselves to block
    // the opponent.
    actions: "block",
    cond: "checkOpponent2InARow",
  },
  {
    // 3. Fork: Create an opportunity where the player
    // has two ways to win (two non-blocked lines of 2).
    actions: "fork",
    cond: "checkMyFork",
  },
...
]

Now, fork here is the heaviest amongst actions and requires us to find an intersection of 2 rows.
In order to check if such an intersection exists (checkMyFork):

  • we have to find all rows/columns/diagonals, with only our symbol in one of their cells
  • find the first intersection, which should be a non-taken spot

If such an intersection does not exist for us - we'll move further down the list of available tactics.
But if such an intersection does indeed exists, - we'll transition to an action fork where we must do the same work finding an intersection all over again.

Of course, there is a workaround for that. You may try to remodel your state in such a way, so you store the result of a condition in a buffer first and then you check on them.
https://github.com/MrNovado/xstate-playground/blob/master/src/feature/TicTacToe/Actor.declarative.2.ts#L143

Which will look something like this:

context: {
    buffer:
        | null
        | { type: "win"; combination: number[] }
        | { type: "blockWin"; combination: number[] }
        | { type: "fork"; intersection: number }
        | { type: "blockFork"; intersection: number }
        | { type: "takeCenter" }
        | { type: "takeOppositeCornter"; corner: number }
        | { type: "takeCorner" }
        | { type: "takeEmptySide" };
},
...,
fork: {
  id: "fork",
  initial: "check",
  states: {
    check: {
      entry: "assignMyFork",
      on: {
        "": [
          {
            cond: ({ buffer }) => buffer ? .type === "fork",
            target: "act",
          },
          {
            target: "#blockFork",
          },
        ],
      },
    },
    act: {
      on: {
        "": {
          target: "#awaitingTurn",
          actions: "fork",
        },
      },
    },
  },
},

Instead of making the same work twice, only assignMyFork now makes the needed computation.
Which works, but actions now have to reason about the buffer in order to make a message:
https://github.com/MrNovado/xstate-playground/blob/master/src/feature/TicTacToe/Actor.declarative.2.ts#L284

fork: sendParent(({ buffer }: DeclarativePerfectActorContext) => {
  switch (buffer?.type) {
    case "fork": {
      return {
        type: "TURN_MADE",
        selectedIndex: buffer.intersection,
      };
    }
    default:
      return {
        type: "UNKNOWN_STATE",
          origin: "declarativePerfectActor2",
          message: "fork is invoked, but buffer is not there!",
      };
  }
}, DELAY),

If only there was a way to restrict the context, make a condition-check and plan an action all in one go.


Thoughts?

@MrNovado
Copy link
Author

MrNovado commented Mar 2, 2020

Probably the easiest way to think about this issue is to consider the current TransitionConfig as a state-model:

{
  cond: true | false,
  actions: action | actions[],
}

By defining cond and actions separately you have this combination of states in the context of executing an action:

exec: (true, action | action[]) | (false, action | action[])  

Where I would argue, that the first variant is overly verbose. If the condition is true, then all we care about on the execution phase is an action, and the true is redundant. So we simplify:

exec: (action | action[]) | (false, action | action[])  

The second variant might be ok since even if the condition is false, we might still want to reason about the action somehow like we might want to report it or whatever. But if we don't need any reporting on an action that won't be executed anyway, we may simplify the case even more:

exec: (action | action[]) | (false)  

So it seems like the (context, event) => action | action[] | false interface is really all you need to describe a transitional-action like that.

@davidkpiano
Copy link
Member

This is what pure() lets you do: https://xstate.js.org/docs/guides/actions.html#pure-action

actions: pure((context, event) => {
  if (someCondition(context)) {
    return [action1, action2];
  }

  return undefined; // no actions
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants