feat: Add endStage and setStage events

[delucis]

Closes #445
Closes #446
Closes #447
🎉

I have tried adding the endStage and setStage events as discussed in #445, #446 and #447.

I’m most of the way there, but ran into a roadblock with adding the moveLimit option for setStage. If I understood correctly (and I’ll admit I felt a little lost reading through all the event-related code), when calling an event from the client, the single passed argument is used as the payload for the action sent to the reducer. So that makes my imagined syntax of setStage('B', { moveLimit: 2 }) not as trivial to implement as I imagined.

Presumably there would need to be a wrapper somewhere that formats arguments to client events so that supporting a second optional argument is possible, but I’m not sure exactly where or how that should be implemented.

In the meantime, I have added a failing test for moveLimit and UpdateStage in flow.js does contain the correct logic to hypothetically update ctx were the moveLimit argument to reach it. If you have any pointers, I can try to implement something, or if you prefer to get this merged without the moveLimit support, I can strip those parts out to be figured out later.

[nicolodavis]

The easiest solution might be to use the short form / long form pattern that we follow elsewhere.

setStage('B')
setStage({ stage: 'B, moveLimit: 2 })

So now you only have one argument.

[delucis]

Oh, right, good idea. I’ll go with that for now :-)

[delucis]

@nicolodavis How should I test for the automatic option to endStage and should it even exist? I’m guessing for endPhase and endTurn this is passed when it is called from endIf, which isn’t currently available for stages.

[nicolodavis]

The only "automatic" end of stage that we have right now is triggered via moveLimit. Maybe have ProcessMove call EndStage and pass automatic: true?

[delucis]

OK!

One thing: right now endStage and setStage operate on currentPlayer only, but moveLimit can remove any active player. I’ll try adding a playerID optional argument that is used internally by ProcessMove.

If I understood correctly, events are only callable by currentPlayer right? What if an active player that is not the current player wants to endStage from a move? Would that work?

[nicolodavis]

If I understood correctly, events are only callable by currentPlayer right? What if an active player that is not the current player wants to endStage from a move? Would that work?

We might have to relax the condition. Maybe let's have any player that is "active" (i.e. one of the activePlayers if it is set OR currentPlayer if it is not set) be able to call events?

[delucis]

let's have any player that is "active" (i.e. one of the activePlayers if it is set OR currentPlayer if it is not set) be able to call events?

That makes sense to me. I guess at some point some config for what events are enabled etc. might be needed, but for now stages are the loosest of all the game elements. (Unlike phases there are no checks that a stage is actually declared when setting it and at the moment they only declare moves.)

I’m not sure exactly how to go about those changes, so I’ll proceed like I suggested with ProcessMove and EndStage as that doesn’t need the changes to events in general.

[nicolodavis]

Thanks again for getting the phases-overhaul effort over the finish line!

[delucis]

It’s been fun slowly figuring out how all the internals work!

ProcessMove now uses EndStage as you suggested — avoids some code duplication too, which is nice.

[delucis]

OK, I think this is good to go. Test coverage is just missing one branch where events.setStage passed to Flow is not undefined. Haven’t fixed that yet because I noticed that at the end of Flow, events.endPhase is used to enable both the endPhase and startPhase events. I copied that pattern for setStage and endStage, but if I’ve understood everything correctly this means that passing

Flow({
  events: {
    setStage: false,
    setPhase: false,
  },
});

will not actually have any effect. So I guess, maybe the logic that sets events.setStage and events.setPhase to true is redundant and should be removed? (All tests still passed when I tried removing them.)

[delucis]

Also, just realised this will resolve everything in #445 #446 & #447 😄 🎉