XXXSQUASHXXX: Iterate on PR feedback (2).

- 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).

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();
+    expect(result).toEqual(
+      expect.not.objectContaining({ transients: expect.anything() })
+    );
+    expect(result).toEqual(
+      expect.not.objectContaining({ stripTransientsResult: expect.anything() })
+    );
+  });
+
+  test('invalid move', () => {
+    const result = client.moves.Invalid();
+    expect(result).toMatchObject({
+      transients: {
+        error: {
+          type: 'action/invalid_move',
+        },
+      },
+    });
+    expect(result).toEqual(
+      expect.objectContaining({ stripTransientsResult: 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';
@@ -42,6 +45,10 @@ type ClientAction =
   | ActionShape.Patch;
 type Action = CredentialedActionShape.Any | ClientAction;
 
+// TODO(#723): Figure out a nice API for the returned ActionResult.
+// For now, let's just return the raw data.
+type ActionResult = any;
+
 export interface DebugOpt {
   target?: HTMLElement;
   impl?: typeof Debug;
@@ -85,7 +92,7 @@ function createDispatchers(
 ) {
   return innerActionNames.reduce((dispatchers, name) => {
     dispatchers[name] = function (...args: any[]) {
-      store.dispatch(
+      return store.dispatch(
         ActionCreators[storeActionType](
           name,
           args,
@@ -96,7 +103,7 @@ function createDispatchers(
     };
 
     return dispatchers;
-  }, {} as Record<string, (...args: any[]) => void>);
+  }, {} as Record<string, (...args: any[]) => ActionResult>);
 }
 
 // Creates a set of dispatchers to make moves.
@@ -148,20 +155,20 @@ export class _ClientImpl<G extends any = any> {
   playerID: PlayerID | null;
   credentials: string;
   matchData?: FilteredMetadata;
-  moves: Record<string, (...args: any[]) => void>;
+  moves: Record<string, (...args: any[]) => ActionResult>;
   events: {
-    endGame?: (gameover?: any) => void;
-    endPhase?: () => void;
-    endTurn?: (arg?: { next: PlayerID }) => void;
-    setPhase?: (newPhase: string) => void;
-    endStage?: () => void;
-    setStage?: (newStage: string) => void;
-    setActivePlayers?: (arg: ActivePlayersArg) => void;
+    endGame?: (gameover?: any) => ActionResult;
+    endPhase?: () => ActionResult;
+    endTurn?: (arg?: { next: PlayerID }) => ActionResult;
+    setPhase?: (newPhase: string) => ActionResult;
+    endStage?: () => ActionResult;
+    setStage?: (newStage: string) => ActionResult;
+    setActivePlayers?: (arg: ActivePlayersArg) => ActionResult;
   };
-  plugins: Record<string, (...args: any[]) => void>;
-  reset: () => void;
-  undo: () => void;
-  redo: () => void;
+  plugins: Record<string, (...args: any[]) => ActionResult>;
+  reset: () => ActionResult;
+  undo: () => ActionResult;
+  redo: () => ActionResult;
   sendChatMessage: (message: any) => void;
   chatMessages: ChatMessage[];
 
@@ -197,21 +204,21 @@ export class _ClientImpl<G extends any = any> {
     }
 
     this.reset = () => {
-      this.store.dispatch(ActionCreators.reset(this.initialState));
+      return this.store.dispatch(ActionCreators.reset(this.initialState));
     };
     this.undo = () => {
       const undo = ActionCreators.undo(
         assumedPlayerID(this.playerID, this.store, this.multiplayer),
         this.credentials
       );
-      this.store.dispatch(undo);
+      return this.store.dispatch(undo);
     };
     this.redo = () => {
       const redo = ActionCreators.redo(
         assumedPlayerID(this.playerID, this.store, this.multiplayer),
         this.credentials
       );
-      this.store.dispatch(redo);
+      return this.store.dispatch(redo);
     };
 
     this.log = [];
@@ -303,6 +310,7 @@ export class _ClientImpl<G extends any = any> {
     };
 
     const middleware = applyMiddleware(
+      TransientHandlingMiddleware,
       SubscriptionMiddleware,
       TransportMiddleware,
       LogMiddleware

src/core/errors.ts

@@ -1,10 +1,18 @@
+/*
+ * 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 = 'match_not_found',
+  MatchNotFound = 'update/match_not_found',
   // Could not apply Patch operation (rfc6902).
-  PatchFailed = 'patch_failed',
+  PatchFailed = 'update/patch_failed',
 }
 
 export enum ActionErrorType {

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),
 };
@@ -136,7 +138,7 @@ test('invalid patch', () => {
     patch(0, 1, [{ op: 'replace', path: '/_stateIDD', value: 1 }], [])
   );
   expect(state).toEqual(originalState);
-  expect(transients.error.type).toEqual('patch_failed');
+  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.
@@ -754,3 +756,36 @@ 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',
+        },
+      },
+    });
+    expect(result).toEqual(
+      expect.objectContaining({ stripTransientsResult: expect.anything() })
+    );
+  });
+});