Repository: boardgameio/boardgame.io (ts)

Issue:
endStage event is processed after a new turn starts
If a move that calls `events.endStage()` triggers the end of the turn (via an `endIf` for example), then that `endStage` event is currently run *after* the set-up for the new turn. This can result in the player who moved last being removed from the `activePlayers` for the new turn.

[Reported on Gitter](https://gitter.im/boardgame-io/General?at=5ff8a4aadc2e4809aa0d9385) by @tomnunn who shared [this CodeSandbox reproduction](https://codesandbox.io/s/bgio-phases-and-stages-u442i).

(This is effectively a duplicate of #769, which reports the inverse problem that an `endIf` doesn’t yet have access to the new `activePlayers`.)
"ctx.events.setPhase('MyPhase')" seems to have issues
I try to [migrate my playing card game](https://github.com/Oliboy50/coinche/pull/11) from boargame.io `0.32.1` to `0.37.2` (first my target was `0.33.0` but it behaves exactly the same).

So I followed the migration guide, but after a lot of debugging, I can't understand why it doesn't work now...

My game is split into several phases (Deal, Talk, PlayCard, CountPoint, etc.) and a lot of logic is done through `onBegin` phase hooks (Deal and CountPoint phases for instance, doesn't contain any move).

Here is a **simple** reproduction of my main issue:
```ts
{
  // ...
  phases: {
    Deal: {
      start: true,
      onBegin: (G, ctx) => {
        console.log('phase Deal');
        ctx.events.setPhase('Talk');
      },
    },
    Talk: {
      onBegin: (G, ctx) => {
        console.log('phase Talk');
        ctx.events.setPhase('PlayCards');
        console.log('TalkEnd');
      },
    },
    PlayCards: {
      onBegin: (G, ctx) => {
        console.log('phase PlayCards');
        ctx.events.setPhase('Talk');
        console.log('PlayCardsEnd');
      },
    },
  },
}
```

then my logs are:
```
phase Deal
phase Talk
TalkEnd
```

it just doesn't want to immediately go to `PlayCards` phase after the `Talk` one and I don't understand why... (it worked fine in version `0.32.1`)

could you help?

(maybe this is relative to https://github.com/nicolodavis/boardgame.io/issues/526)
endTurn inside onBegin doesn't work
```js
{
  turn: {
    onBegin: (G, ctx) => {
      if (ctx.turn < 10) ctx.events.endTurn();    // this is a no-op
    }
  }
}
```

This [check](https://github.com/nicolodavis/boardgame.io/blob/master/src/core/events.js#L49) prevents the `endTurn` because the `Events` class is attached at a point where the turn was 0 [here](https://github.com/nicolodavis/boardgame.io/blob/master/src/core/initialize.js#L36), but by the time the event is processed the turn is incremented to 1.

Pull request description:
feat(events): Don’t leak stage events across turns & allow self-ending turns/phases
Closes #899
Closes #526
Closes #558

An important side effect of this change is that it opens up the possibility to produce infinite loops using effects on hooks. We should add some check to prevent this, or at least warn about it.

We should also evaluate the possibility of remove ctx from endIf, because the developers expect its value to be up-to-date and it can be confusing.

---

**Edit (by @delucis):** This does not yet address #769 because that needs `endIf` to run _after_ `activePlayers` has been updated.

Patch:
diff --git a/src/plugins/events/events.ts b/src/plugins/events/events.ts
index 83a420c..341f48c 100644
--- a/src/plugins/events/events.ts
+++ b/src/plugins/events/events.ts
@@ -23,6 +23,7 @@ export interface EventsAPI {
 export interface PrivateEventsAPI {
   _obj: {
     isUsed(): boolean;
+    updateTurnContext(ctx: Ctx): void;
     update(state: State): State;
   };
 }
@@ -34,31 +35,39 @@ export class Events {
   flow: Game['flow'];
   playerID: PlayerID | undefined;
   dispatch: Array<{
-    key: string;
+    type: string;
     args: any[];
     phase: string;
     turn: number;
   }>;
+  maxEndedTurnsPerAction: number;
+  initialTurn: number;
+  currentPhase: string;
+  currentTurn: number;
 
-  constructor(flow: Game['flow'], playerID?: PlayerID) {
+  constructor(flow: Game['flow'], ctx: Ctx, playerID?: PlayerID) {
     this.flow = flow;
     this.playerID = playerID;
     this.dispatch = [];
+    this.initialTurn = ctx.turn;
+    this.updateTurnContext(ctx);
+    // This is an arbitrarily large upper threshold, which could be made
+    // configurable via a game option if the need arises.
+    this.maxEndedTurnsPerAction = ctx.numPlayers * 100;
   }
 
-  /**
-   * Attaches the Events API to ctx.
-   * @param {object} ctx - The ctx object to attach to.
-   */
-  api(ctx: Ctx) {
+  api() {
     const events: EventsAPI & PrivateEventsAPI = {
       _obj: this,
     };
-    const { phase, turn } = ctx;
-
-    for (const key of this.flow.eventNames) {
-      events[key] = (...args: any[]) => {
-        this.dispatch.push({ key, args, phase, turn });
+    for (const type of this.flow.eventNames) {
+      events[type] = (...args: any[]) => {
+        this.dispatch.push({
+          type,
+          args,
+          phase: this.currentPhase,
+          turn: this.currentTurn,
+        });
       };
     }
 
@@ -69,30 +78,70 @@ export class Events {
     return this.dispatch.length > 0;
   }
 
+  updateTurnContext(ctx: Ctx) {
+    this.currentPhase = ctx.phase;
+    this.currentTurn = ctx.turn;
+  }
+
+  stateWithError(state: State): State {
+    return {
+      ...state,
+      plugins: {
+        ...state.plugins,
+        events: {
+          ...state.plugins.events,
+          data: {
+            error:
+              'Maximum number of turn endings exceeded for this update.\n' +
+              'This likely means game code is triggering an infinite loop.',
+          },
+        },
+      },
+    };
+  }
+
   /**
    * Updates ctx with the triggered events.
    * @param {object} state - The state object { G, ctx }.
    */
-  update(state: State) {
+  update(state: State): State {
+    const initialState = state;
     for (let i = 0; i < this.dispatch.length; i++) {
-      const item = this.dispatch[i];
+      const endedTurns = this.currentTurn - this.initialTurn;
+      // This protects against potential infinite loops if specific events are called on hooks.
+      // The moment we exceed the defined threshold, we just bail out of all phases.
+      if (endedTurns >= this.maxEndedTurnsPerAction) {
+        return this.stateWithError(initialState);
+      }
+
+      const event = this.dispatch[i];
+
+      // If the turn already ended, don't try to process stage events.
+      if (
+        (event.type === 'endStage' ||
+          event.type === 'setStage' ||
+          event.type === 'setActivePlayers') &&
+        event.turn !== state.ctx.turn
+      ) {
+        continue;
+      }
 
       // If the turn already ended some other way,
       // don't try to end the turn again.
-      if (item.key === 'endTurn' && item.turn !== state.ctx.turn) {
+      if (event.type === 'endTurn' && event.turn !== state.ctx.turn) {
         continue;
       }
 
       // If the phase already ended some other way,
       // don't try to end the phase again.
       if (
-        (item.key === 'endPhase' || item.key === 'setPhase') &&
-        item.phase !== state.ctx.phase
+        (event.type === 'endPhase' || event.type === 'setPhase') &&
+        event.phase !== state.ctx.phase
       ) {
         continue;
       }
 
-      const action = automaticGameEvent(item.key, item.args, this.playerID);
+      const action = automaticGameEvent(event.type, event.args, this.playerID);
 
       state = {
         ...state,
diff --git a/src/plugins/plugin-events.ts b/src/plugins/plugin-events.ts
index 380e3a1..a81efea 100644
--- a/src/plugins/plugin-events.ts
+++ b/src/plugins/plugin-events.ts
@@ -15,17 +15,25 @@ export type { EventsAPI };
 const EventsPlugin: Plugin<EventsAPI & PrivateEventsAPI> = {
   name: 'events',
 
-  noClient: ({ api }) => {
-    return api._obj.isUsed();
-  },
+  noClient: ({ api }) => api._obj.isUsed(),
 
-  dangerouslyFlushRawState: ({ state, api }) => {
-    return api._obj.update(state);
-  },
+  isInvalid: ({ data }) => data.error || false,
 
-  api: ({ game, playerID, ctx }) => {
-    return new Events(game.flow, playerID).api(ctx);
-  },
+  // Update the events plugin’s internal turn context each time a move
+  // or hook is called. This allows events called after turn or phase
+  // endings to dispatch the current turn and phase correctly.
+  fnWrap:
+    (fn) =>
+    (G, ctx, ...args) => {
+      const api = ctx.events as PrivateEventsAPI;
+      if (api) api._obj.updateTurnContext(ctx);
+      G = fn(G, ctx, ...args);
+      return G;
+    },
+
+  dangerouslyFlushRawState: ({ state, api }) => api._obj.update(state),
+
+  api: ({ game, ctx, playerID }) => new Events(game.flow, ctx, playerID).api(),
 };
 
 export default EventsPlugin;


Test patch:
diff --git a/src/core/flow.test.ts b/src/core/flow.test.ts
index 8678b63..238a249 100644
--- a/src/core/flow.test.ts
+++ b/src/core/flow.test.ts
@@ -16,6 +16,7 @@ jest.mock('../core/logger', () => ({
   info: jest.fn(),
   error: jest.fn(),
 }));
+afterEach(jest.clearAllMocks);
 
 describe('phases', () => {
   test('invalid phase name', () => {
@@ -957,7 +958,7 @@ describe('moveMap', () => {
 });
 
 describe('infinite loops', () => {
-  test('loop 1', () => {
+  test('infinite loop of self-ending phases via endIf', () => {
     const endIf = () => true;
     const game = {
       phases: {
@@ -969,12 +970,49 @@ describe('infinite loops', () => {
     expect(client.getState().ctx.phase).toBe(null);
   });
 
-  test('loop 2', () => {
+  test('infinite endPhase loop from phase.onBegin', () => {
+    const onBegin = (G, ctx) => ctx.events.endPhase();
     const game = {
-      turn: {
-        onEnd: (G, ctx) => {
-          ctx.events.endPhase();
+      phases: {
+        A: {
+          onBegin,
+          next: 'B',
+          start: true,
+          moves: {
+            a: (G, ctx) => void ctx.events.endPhase(),
+          },
         },
+        B: { onBegin, next: 'C' },
+        C: { onBegin, next: 'A' },
+      },
+    };
+
+    // The onBegin fails to end the phase during initialisation.
+    const client = Client({ game, numPlayers: 3 });
+    let state = client.getState();
+    expect(state.ctx.phase).toBe('A');
+    expect(state.ctx.turn).toBe(1);
+    expect(error).toHaveBeenCalled();
+    expect((error as jest.Mock).mock.calls[0][0]).toMatch(
+      /events plugin declared action invalid/
+    );
+    jest.clearAllMocks();
+
+    // Moves also fail because of the infinite loop (the game is stuck).
+    client.moves.a();
+    state = client.getState();
+    expect(error).toHaveBeenCalled();
+    expect((error as jest.Mock).mock.calls[0][0]).toMatch(
+      /events plugin declared action invalid/
+    );
+    expect(state.ctx.phase).toBe('A');
+    expect(state.ctx.turn).toBe(1);
+  });
+
+  test('double phase ending from client event and turn.onEnd', () => {
+    const game = {
+      turn: {
+        onEnd: (G, ctx) => void ctx.events.endPhase(),
       },
       phases: {
         A: { next: 'B', start: true },
@@ -984,12 +1022,17 @@ describe('infinite loops', () => {
     };
     const client = Client({ game });
 
-    expect(client.getState().ctx.phase).toBe('A');
+    let state = client.getState();
+    expect(state.ctx.phase).toBe('A');
+    expect(state.ctx.turn).toBe(1);
     client.events.endPhase();
-    expect(client.getState().ctx.phase).toBe('B');
+
+    state = client.getState();
+    expect(state.ctx.phase).toBe('B');
+    expect(state.ctx.turn).toBe(2);
   });
 
-  test('loop 3', () => {
+  test('infinite turn endings from turn.onBegin', () => {
     const game = {
       moves: {
         endTurn: (G, ctx) => {
@@ -997,16 +1040,26 @@ describe('infinite loops', () => {
         },
       },
       turn: {
-        onBegin: (G, ctx) => ctx.events.endTurn(),
+        onBegin: (G, ctx) => void ctx.events.endTurn(),
       },
     };
     const client = Client({ game });
+    const initialState = client.getState();
     expect(client.getState().ctx.currentPlayer).toBe('0');
+
+    // Trigger infinite loop
     client.moves.endTurn();
-    expect(client.getState().ctx.currentPlayer).toBe('1');
+
+    // Expect state to be unchanged and error to be logged.
+    expect(error).toHaveBeenCalled();
+    expect((error as jest.Mock).mock.calls[0][0]).toMatch(
+      /events plugin declared action invalid/
+    );
+    expect(client.getState().ctx.currentPlayer).toBe('0');
+    expect(client.getState()).toEqual({ ...initialState, deltalog: [] });
   });
 
-  test('loop 4', () => {
+  test('double turn ending from event and endIf', () => {
     const game = {
       moves: {
         endTurn: (G, ctx) => {
@@ -1018,9 +1071,257 @@ describe('infinite loops', () => {
       },
     };
     const client = Client({ game });
-    expect(client.getState().ctx.currentPlayer).toBe('0');
+
+    // turn.endIf is ignored during game setup.
+    let state = client.getState();
+    expect(state.ctx.currentPlayer).toBe('0');
+    expect(state.ctx.turn).toBe(1);
+
+    // turn.endIf is ignored when the turn was just ended.
     client.moves.endTurn();
-    expect(client.getState().ctx.currentPlayer).toBe('1');
+    state = client.getState();
+    expect(state.ctx.currentPlayer).toBe('1');
+    expect(state.ctx.turn).toBe(2);
+  });
+});
+
+describe('events in hooks', () => {
+  const moves = {
+    setAutoEnd: () => ({ shouldEnd: true }),
+  };
+
+  describe('endTurn', () => {
+    const conditionalEndTurn = (G, ctx) => {
+      if (!G.shouldEnd) return;
+      G.shouldEnd = false;
+      ctx.events.endTurn();
+    };
+
+    test('can end turn from turn.onBegin', () => {
+      const client = Client({
+        game: { moves, turn: { onBegin: conditionalEndTurn } },
+      });
+
+      client.moves.setAutoEnd();
+
+      let state = client.getState();
+      expect(state.ctx.turn).toBe(1);
+      expect(state.ctx.currentPlayer).toBe('0');
+
+      client.events.endTurn();
+      state = client.getState();
+      expect(state.ctx.turn).toBe(3);
+      expect(state.ctx.currentPlayer).toBe('0');
+    });
+
+    test('cannot end turn from phase.onBegin', () => {
+      const client = Client({
+        game: {
+          moves,
+          phases: {
+            A: { onBegin: conditionalEndTurn },
+          },
+        },
+      });
+
+      client.moves.setAutoEnd();
+
+      let state = client.getState();
+      expect(state.ctx.turn).toBe(1);
+      expect(state.ctx.currentPlayer).toBe('0');
+      expect(state.ctx.phase).toBeNull();
+
+      client.events.setPhase('A');
+      state = client.getState();
+      expect(state.ctx.turn).toBe(2);
+      expect(state.ctx.currentPlayer).toBe('1');
+      expect(state.ctx.phase).toBe('A');
+    });
+
+    test('can end turn from turn.onBegin at start of phase', () => {
+      const client = Client({
+        game: {
+          moves,
+          phases: {
+            A: {
+              turn: { onBegin: conditionalEndTurn },
+            },
+          },
+        },
+      });
+
+      client.moves.setAutoEnd();
+
+      let state = client.getState();
+      expect(state.ctx.phase).toBeNull();
+      expect(state.ctx.turn).toBe(1);
+      expect(state.ctx.currentPlayer).toBe('0');
+
+      client.events.setPhase('A');
+      state = client.getState();
+      expect(state.ctx.phase).toBe('A');
+      expect(state.ctx.turn).toBe(3);
+      expect(state.ctx.currentPlayer).toBe('0');
+    });
+
+    test('cannot end turn from turn.onEnd', () => {
+      const client = Client({
+        game: {
+          moves,
+          turn: { onEnd: conditionalEndTurn },
+        },
+      });
+
+      let state = client.getState();
+      expect(state.ctx.turn).toBe(1);
+      expect(state.ctx.currentPlayer).toBe('0');
+
+      client.moves.setAutoEnd();
+      client.events.endTurn();
+      state = client.getState();
+      expect(state.ctx.turn).toBe(2);
+      expect(state.ctx.currentPlayer).toBe('1');
+    });
+
+    test('cannot end turn from phase.onEnd', () => {
+      const client = Client({
+        game: {
+          moves,
+          phases: {
+            A: {
+              start: true,
+              onEnd: conditionalEndTurn,
+            },
+          },
+        },
+      });
+
+      let state = client.getState();
+      expect(state.ctx.turn).toBe(1);
+      expect(state.ctx.currentPlayer).toBe('0');
+      expect(state.ctx.phase).toBe('A');
+
+      client.moves.setAutoEnd();
+      client.events.endPhase();
+      state = client.getState();
+      expect(state.ctx.turn).toBe(2);
+      expect(state.ctx.currentPlayer).toBe('1');
+      expect(state.ctx.phase).toBeNull();
+    });
+  });
+
+  describe('endPhase', () => {
+    const conditionalEndPhase = (G, ctx) => {
+      if (!G.shouldEnd) return;
+      G.shouldEnd = false;
+      ctx.events.endPhase();
+    };
+
+    test('can end phase from turn.onBegin', () => {
+      const client = Client({
+        game: {
+          moves,
+          phases: {
+            A: {
+              start: true,
+              turn: { onBegin: conditionalEndPhase },
+            },
+          },
+        },
+      });
+
+      let state = client.getState();
+      expect(state.ctx.turn).toBe(1);
+      expect(state.ctx.currentPlayer).toBe('0');
+      expect(state.ctx.phase).toBe('A');
+
+      client.moves.setAutoEnd();
+      client.events.endTurn();
+      state = client.getState();
+      expect(state.ctx.turn).toBe(3);
+      expect(state.ctx.currentPlayer).toBe('0');
+      expect(state.ctx.phase).toBeNull();
+    });
+
+    test('can end phase from phase.onBegin', () => {
+      const client = Client({
+        game: {
+          moves,
+          phases: {
+            A: { onBegin: conditionalEndPhase },
+          },
+        },
+      });
+
+      let state = client.getState();
+      expect(state.ctx.turn).toBe(1);
+      expect(state.ctx.currentPlayer).toBe('0');
+      expect(state.ctx.phase).toBeNull();
+
+      client.moves.setAutoEnd();
+      client.events.setPhase('A');
+      state = client.getState();
+      expect(state.ctx.turn).toBe(3);
+      expect(state.ctx.currentPlayer).toBe('0');
+      expect(state.ctx.phase).toBeNull();
+    });
+
+    test('can end phase from turn.onEnd', () => {
+      const client = Client({
+        game: {
+          moves,
+          phases: {
+            A: {
+              start: true,
+              turn: { onEnd: conditionalEndPhase },
+            },
+          },
+        },
+      });
+
+      let state = client.getState();
+      expect(state.ctx.turn).toBe(1);
+      expect(state.ctx.currentPlayer).toBe('0');
+      expect(state.ctx.phase).toBe('A');
+
+      client.moves.setAutoEnd();
+      client.events.endTurn();
+      state = client.getState();
+      // TODO: This is likely not the desired behaviour. Turn 1 is ended,
+      // then the phase is ended, automatically ending turn 2, ending up in turn 3.
+      // Turn 2 is effectively skipped. Works better with TurnOrder.CONTINUE.
+      expect(state.ctx.turn).toBe(3);
+      expect(state.ctx.currentPlayer).toBe('0');
+      expect(state.ctx.phase).toBeNull();
+    });
+
+    test('cannot end phase from phase.onEnd', () => {
+      const client = Client({
+        game: {
+          moves,
+          phases: {
+            A: {
+              start: true,
+              next: 'B',
+              onEnd: conditionalEndPhase,
+            },
+            B: {},
+          },
+        },
+      });
+
+      let state = client.getState();
+      expect(state.ctx.turn).toBe(1);
+      expect(state.ctx.currentPlayer).toBe('0');
+      expect(state.ctx.phase).toBe('A');
+
+      client.moves.setAutoEnd();
+      client.events.endPhase();
+      state = client.getState();
+      expect(state.ctx.turn).toBe(2);
+      expect(state.ctx.currentPlayer).toBe('1');
+      expect(state.ctx.phase).toBe('B');
+    });
   });
 });
 
@@ -1084,3 +1385,56 @@ test('events in hooks triggered by moves should be processed', () => {
     '1': 'A',
   });
 });
+
+test('stage events should not be processed out of turn', () => {
+  const game = {
+    phases: {
+      A: {
+        start: true,
+        turn: {
+          activePlayers: {
+            all: 'A1',
+          },
+          stages: {
+            A1: {
+              moves: {
+                endStage: (G, ctx) => {
+                  G.endStage = true;
+                  ctx.events.endStage();
+                },
+              },
+            },
+          },
+        },
+        endIf: (G) => G.endStage,
+        next: 'B',
+      },
+      B: {
+        turn: {
+          activePlayers: {
+            all: 'B1',
+          },
+          stages: {
+            B1: {},
+          },
+        },
+      },
+    },
+  };
+
+  const client = Client({ game, numPlayers: 3 });
+
+  expect(client.getState().ctx.activePlayers).toEqual({
+    '0': 'A1',
+    '1': 'A1',
+    '2': 'A1',
+  });
+
+  client.moves.endStage();
+
+  expect(client.getState().ctx.activePlayers).toEqual({
+    '0': 'B1',
+    '1': 'B1',
+    '2': 'B1',
+  });
+});
diff --git a/src/plugins/events/events.test.ts b/src/plugins/events/events.test.ts
index 1db1b72..3b05177 100644
--- a/src/plugins/events/events.test.ts
+++ b/src/plugins/events/events.test.ts
@@ -13,23 +13,26 @@ import type { Game, Ctx } from '../../types';
 test('constructor', () => {
   const flow = {} as Game['flow'];
   const playerID = '0';
-  const e = new Events(flow, playerID);
+  const e = new Events(flow, { phase: '', turn: 0 } as Ctx, playerID);
   expect(e.flow).toBe(flow);
   expect(e.playerID).toBe(playerID);
   expect(e.dispatch).toEqual([]);
+  expect(e.initialTurn).toEqual(0);
+  expect(e.currentPhase).toEqual('');
+  expect(e.currentTurn).toEqual(0);
 });
 
 test('dispatch', () => {
   const flow = { eventNames: ['A', 'B'] } as Game['flow'];
-  const e = new Events(flow);
-  const events = e.api({ phase: '', turn: 0 } as Ctx);
+  const e = new Events(flow, { phase: '', turn: 0 } as Ctx);
+  const events = e.api();
 
   expect(e.dispatch).toEqual([]);
   (events as unknown as { A(): void }).A();
   (events as unknown as { B(): void }).B();
   expect(e.dispatch).toEqual([
-    { key: 'A', args: [], phase: '', turn: 0 },
-    { key: 'B', args: [], phase: '', turn: 0 },
+    { type: 'A', args: [], phase: '', turn: 0 },
+    { type: 'B', args: [], phase: '', turn: 0 },
   ]);
 });