moveLimit overrides the next parameter in endTurn

[JoeLudwig]

To reproduce, create a game with this in its declaration:

    turn: 
    {
        moveLimit: 1,
    },

And then in a move, fire the endTurn event with a next argument:

        ctx.events?.endTurn?.( { next: '2' } );

Play will proceed to the next player in the play order instead of the next player specified in endTurn. I would guess the endTurn event fired from the move is actually doing nothing, but I don't know for sure. If I remove the moveLimit from the game it works as expected, so maybe the bug here is that endTurn doesn't report any kind of error to indicate that it's not going to do anything?

[delucis]

Right, because events are pushed to a queue to be processed later, the sequence is:

1. Run any code in the move (mutating G etc.). Any events called are stored for later.

2. Check if the turn needs ending because moveLimit has been reached and end it if it does.

3. Process the events. Because the turn has already ended, the framework skips what it assumes is an excess endTurn request.

I guess there are a few potential ways to resolve this:

1. Process events before the moveLimit check, so next would work as expected and the moveLimit turn end would be skipped. (I suspect this might be architecturally fiddly, if I remember the logic flow correctly, but could be nice if it doesn’t have other side effects.)

2. Keep the current behaviour, but log a warning that it may be programmer error to end the turn in both these ways as you suggested. I don’t think we’d know exactly why the turn was ending twice at the moment, but we could log a warning here: https://github.com/boardgameio/boardgame.io/blob/23734381ba610d7162f5cf4c34b3c88c07a4fe3f/src/core/flow.ts#L482-L486

3. Remove that check blocking multiple turn ends, so that this combo actually ends two turns causing more confusion, but forcing a programmer to decide if they want to use moveLimit or events 😆

[JoeLudwig]

I certainly would have noticed if I had ended two turns the entire time. :)

I don't know this codebase at all, so this might not work, but could you have moveLimit push an endTurn event instead of running the endTurn logic directly? That way the "discard duplicates" code above kicks in, but it prefers the first endTurn from the move handler instead of the automatic one from moveLimit.