feat: Improve client error handling [Part 1] (#940)

* feature(#723): Improve client error handling [Part 1]

This PR sets up the initial reducer/master plumbing for error handling
by introducing a "transients" option bag containing errors.

The goal of this change is to be as backwards-compatible as possible,
with subsequent changes building on top of the API extensions added
here.

Notes:

- Previously, there was an assumption that the state resulting from a
reducer processing an action is suitable for client consumption.
- Now, transient artifacts might be appended onto the state and need to
be stripped before being sent to the client.
- To work around this change, I added some new types to help signal
the "transient" nature of State that enters and exits the reducer.
- I'm assuming the master is the single caller of the core reducer and
leaving it up to the master to do this stripping.

* XXXSQUASHXXX: Iterate on PR feedback (3).
- Fix typo in error codes
- Fix typo in optional type.
- Strip transients early in the reducer, obviating type changes to the
plugins/main code.
- Unify the re-dispatch-on-transient pattern between master and client
with a new middleware.
- Fix some fragile tests (See #941).
- Narrow client/master test changes for new functionality. Left some
TODO/dev notes for things to be done in subsequent PRs for #723 and for
hypothetical future discussion.
- Remove currently unused ActionResult to avoid extraneous diffs.

* Expand union type to include undefined

Co-authored-by: Chris Swithinbank <[email protected]>

* Update error types for action_invalid.

Co-authored-by: Chris Swithinbank <[email protected]>

Author: shaoster

src/client/client.test.ts

@@ -6,6 +6,7 @@
  * https://opensource.org/licenses/MIT.
  */
 
+import { INVALID_MOVE } from '../core/constants';
 import { createStore } from 'redux';
 import { CreateGameReducer } from '../core/reducer';
 import { InitializeGame } from '../core/initialize';
@@ -498,6 +499,45 @@ describe('move dispatchers', () => {
   });
 });
 
+describe('transient handling', () => {
+  let client = null;
+
+  beforeEach(() => {
+    client = Client({
+      game: {
+        moves: {
+          A: () => ({}),
+          Invalid: () => INVALID_MOVE,
+        },
+      },
+    });
+  });
+
+  test('regular move', () => {
+    const result = client.moves.A();
+    // TODO(#723): Check against a successful ActionResult.
+    expect(result).toBeUndefined();
+    const state = client.store.getState();
+    // Slightly paranoid check to ensure we don't erroneously add transients.
+    expect(state).toEqual(
+      expect.not.objectContaining({ transients: expect.anything() })
+    );
+  });
+
+  test('invalid move', () => {
+    const result = client.moves.Invalid();
+    // TODO(#723): Check against an errored ActionResult.
+    expect(result).toBeUndefined();
+    const state = client.store.getState();
+    // Ensure we've stripped the transients automagically.
+    // At the time this test was written, this effectively ensures that Client
+    // hooks up the TransientHandlingMiddleware correctly.
+    expect(state).toEqual(
+      expect.not.objectContaining({ transients: expect.anything() })
+    );
+  });
+});
+
 describe('log handling', () => {
   let client = null;
 
@@ -769,52 +809,62 @@ test('override game state', () => {
   expect(client.getState().G).toEqual({ moved: true });
 });
 
+// TODO(#941): These tests should validate DOM mounting/unmounting.
 describe('start / stop', () => {
+  beforeEach(() => {
+    // Don't let other calls to `error` pollute this state.
+    jest.resetAllMocks();
+  });
+
   test('mount on custom element', () => {
     const el = document.createElement('div');
     const client = Client({ game: {}, debug: { target: el } });
-    client.start();
-    client.stop();
-    expect(error).toHaveBeenCalledTimes(3);
-    expect(error).toHaveBeenCalledWith('disallowed move: B');
-    expect(error).toHaveBeenCalledWith('disallowed move: C');
-    expect(error).toHaveBeenLastCalledWith('disallowed move: A');
+    expect(() => {
+      client.start();
+      client.stop();
+    }).not.toThrow();
+    expect(error).not.toHaveBeenCalled();
   });
 
   test('no error when mounting on null element', () => {
     const client = Client({ game: {}, debug: { target: null } }) as any;
+    expect(() => {
+      client.start();
+      client.stop();
+    }).not.toThrow();
+
     client.start();
     client.stop();
     expect(client.manager.debugPanel).toBe(null);
   });
 
   test('override debug implementation', () => {
     const client = Client({ game: {}, debug: { impl: Debug } });
+    expect(() => {
+      client.start();
+      client.stop();
+    }).not.toThrow();
+
     client.start();
     client.stop();
-    expect(error).toHaveBeenCalledTimes(3);
-    expect(error).toHaveBeenCalledWith('disallowed move: B');
-    expect(error).toHaveBeenCalledWith('disallowed move: C');
-    expect(error).toHaveBeenLastCalledWith('disallowed move: A');
+    expect(error).not.toHaveBeenCalled();
   });
 
   test('production mode', () => {
     process.env.NODE_ENV = 'production';
     const client = Client({ game: {} });
-    client.start();
-    client.stop();
-    expect(error).toHaveBeenCalledTimes(3);
-    expect(error).toHaveBeenCalledWith('disallowed move: B');
-    expect(error).toHaveBeenCalledWith('disallowed move: C');
-    expect(error).toHaveBeenLastCalledWith('disallowed move: A');
+    expect(() => {
+      client.start();
+      client.stop();
+    }).not.toThrow();
+    expect(error).not.toHaveBeenCalled();
   });
 
   test('try to stop without starting', () => {
     const client = Client({ game: {} });
-    client.stop();
-    expect(error).toHaveBeenCalledTimes(3);
-    expect(error).toHaveBeenCalledWith('disallowed move: B');
-    expect(error).toHaveBeenCalledWith('disallowed move: C');
-    expect(error).toHaveBeenLastCalledWith('disallowed move: A');
+    expect(() => {
+      client.stop();
+    }).not.toThrow();
+    expect(error).not.toHaveBeenCalled();
   });
 });

src/client/client.ts

@@ -14,7 +14,10 @@ import * as Actions from '../core/action-types';
 import * as ActionCreators from '../core/action-creators';
 import { ProcessGameConfig } from '../core/game';
 import type Debug from './debug/Debug.svelte';
-import { CreateGameReducer } from '../core/reducer';
+import {
+  CreateGameReducer,
+  TransientHandlingMiddleware,
+} from '../core/reducer';
 import { InitializeGame } from '../core/initialize';
 import { PlayerView } from '../plugins/main';
 import type { Transport, TransportOpts } from './transport/transport';
@@ -303,6 +306,7 @@ export class _ClientImpl<G extends any = any> {
     };
 
     const middleware = applyMiddleware(
+      TransientHandlingMiddleware,
       SubscriptionMiddleware,
       TransportMiddleware,
       LogMiddleware

src/core/action-creators.ts

@@ -149,3 +149,11 @@ export const plugin = (
   type: Actions.PLUGIN as typeof Actions.PLUGIN,
   payload: { type, args, playerID, credentials },
 });
+
+/**
+ * Private action used to strip transient metadata (e.g. errors) from the game
+ * state.
+ */
+export const stripTransients = () => ({
+  type: Actions.STRIP_TRANSIENTS as typeof Actions.STRIP_TRANSIENTS,
+});

src/core/action-types.ts

@@ -15,3 +15,4 @@ export const UNDO = 'UNDO';
 export const UPDATE = 'UPDATE';
 export const PATCH = 'PATCH';
 export const PLUGIN = 'PLUGIN';
+export const STRIP_TRANSIENTS = 'STRIP_TRANSIENTS';

src/core/errors.ts

@@ -0,0 +1,33 @@
+/*
+ * Copyright 2017 The boardgame.io Authors
+ *
+ * Use of this source code is governed by a MIT-style
+ * license that can be found in the LICENSE file or at
+ * https://opensource.org/licenses/MIT.
+ */
+
+export enum UpdateErrorType {
+  // The action’s credentials were missing or invalid
+  UnauthorizedAction = 'update/unauthorized_action',
+  // The action’s matchID was not found
+  MatchNotFound = 'update/match_not_found',
+  // Could not apply Patch operation (rfc6902).
+  PatchFailed = 'update/patch_failed',
+}
+
+export enum ActionErrorType {
+  // The action contained a stale state ID
+  StaleStateId = 'action/stale_state_id',
+  // The requested move is unknown or not currently available
+  UnavailableMove = 'action/unavailable_move',
+  // The move declared it was invalid (INVALID_MOVE constant)
+  InvalidMove = 'action/invalid_move',
+  // The player making the action is not currently active
+  InactivePlayer = 'action/inactive_player',
+  // The game has finished
+  GameOver = 'action/gameover',
+  // The requested action is disabled (e.g. undo/redo, events)
+  ActionDisabled = 'action/action_disabled',
+  // The requested action is not currently possible
+  ActionInvalid = 'action/action_invalid',
+}

src/core/reducer.test.ts

@@ -7,7 +7,8 @@
  */
 
 import { INVALID_MOVE } from './constants';
-import { CreateGameReducer } from './reducer';
+import { applyMiddleware, createStore } from 'redux';
+import { CreateGameReducer, TransientHandlingMiddleware } from './reducer';
 import { InitializeGame } from './initialize';
 import {
   makeMove,
@@ -32,6 +33,7 @@ const game: Game = {
     A: (G) => G,
     B: () => ({ moved: true }),
     C: () => ({ victory: true }),
+    Invalid: () => INVALID_MOVE,
   },
   endIf: (G, ctx) => (G.victory ? ctx.currentPlayer : undefined),
 };
@@ -131,11 +133,16 @@ test('valid patch', () => {
 
 test('invalid patch', () => {
   const originalState = { _stateID: 0, G: 'patch' } as State;
-  const state = reducer(
+  const { transients, ...state } = reducer(
     originalState,
     patch(0, 1, [{ op: 'replace', path: '/_stateIDD', value: 1 }], [])
   );
   expect(state).toEqual(originalState);
+  expect(transients.error.type).toEqual('update/patch_failed');
+  // It's an array.
+  expect(transients.error.payload.length).toEqual(1);
+  // It looks like the standard rfc6902 error language.
+  expect(transients.error.payload[0].toString()).toContain('/_stateIDD');
 });
 
 test('reset', () => {
@@ -431,7 +438,15 @@ describe('undo / redo', () => {
 
   test('redo only resets deltalog if nothing to redo', () => {
     const state = reducer(initialState, makeMove('move', 'A', '0'));
-    expect(reducer(state, redo())).toEqual({ ...state, deltalog: [] });
+    expect(reducer(state, redo())).toMatchObject({
+      ...state,
+      deltalog: [],
+      transients: {
+        error: {
+          type: 'action/action_invalid',
+        },
+      },
+    });
   });
 });
 
@@ -526,13 +541,29 @@ describe('undo stack', () => {
 
   test('can’t undo at the start of a turn', () => {
     const newState = reducer(state, undo());
-    expect(newState).toEqual({ ...state, deltalog: [] });
+    expect(newState).toMatchObject({
+      ...state,
+      deltalog: [],
+      transients: {
+        error: {
+          type: 'action/action_invalid',
+        },
+      },
+    });
   });
 
   test('can’t undo another player’s move', () => {
     state = reducer(state, makeMove('basic', null, '1'));
     const newState = reducer(state, undo('0'));
-    expect(newState).toEqual({ ...state, deltalog: [] });
+    expect(newState).toMatchObject({
+      ...state,
+      deltalog: [],
+      transients: {
+        error: {
+          type: 'action/action_invalid',
+        },
+      },
+    });
   });
 });
 
@@ -588,7 +619,15 @@ describe('redo stack', () => {
     expect(state._redo).toHaveLength(1);
     const newState = reducer(state, redo('0'));
     expect(state._redo).toHaveLength(1);
-    expect(newState).toEqual({ ...state, deltalog: [] });
+    expect(newState).toMatchObject({
+      ...state,
+      deltalog: [],
+      transients: {
+        error: {
+          type: 'action/action_invalid',
+        },
+      },
+    });
   });
 });
 
@@ -772,3 +811,33 @@ describe('undo / redo with stages', () => {
     expect(state.ctx.activePlayers['0']).toBe('B');
   });
 });
+
+describe('TransientHandlingMiddleware', () => {
+  const middleware = applyMiddleware(TransientHandlingMiddleware);
+  let store = null;
+
+  beforeEach(() => {
+    store = createStore(reducer, initialState, middleware);
+  });
+
+  test('regular dispatch result has no transients', () => {
+    const result = store.dispatch(makeMove('A'));
+    expect(result).toEqual(
+      expect.not.objectContaining({ transients: expect.anything() })
+    );
+    expect(result).toEqual(
+      expect.not.objectContaining({ stripTransientsResult: expect.anything() })
+    );
+  });
+
+  test('failing dispatch result contains transients', () => {
+    const result = store.dispatch(makeMove('Invalid'));
+    expect(result).toMatchObject({
+      transients: {
+        error: {
+          type: 'action/invalid_move',
+        },
+      },
+    });
+  });
+});