diff --git a/src/client/client.test.ts b/src/client/client.test.ts index 9fca2838c..7c5b54e79 100644 --- a/src/client/client.test.ts +++ b/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,20 +809,30 @@ 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); @@ -790,31 +840,31 @@ describe('start / stop', () => { 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(); }); }); diff --git a/src/client/client.ts b/src/client/client.ts index 521af2040..5f0726544 100644 --- a/src/client/client.ts +++ b/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 { }; const middleware = applyMiddleware( + TransientHandlingMiddleware, SubscriptionMiddleware, TransportMiddleware, LogMiddleware diff --git a/src/core/action-creators.ts b/src/core/action-creators.ts index 1834118c9..48d479ced 100644 --- a/src/core/action-creators.ts +++ b/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, +}); diff --git a/src/core/action-types.ts b/src/core/action-types.ts index d282c394c..dbc83a659 100644 --- a/src/core/action-types.ts +++ b/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'; diff --git a/src/core/errors.ts b/src/core/errors.ts new file mode 100644 index 000000000..267c8621d --- /dev/null +++ b/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', +} diff --git a/src/core/reducer.test.ts b/src/core/reducer.test.ts index 02d946195..0d2c83917 100644 --- a/src/core/reducer.test.ts +++ b/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', + }, + }, + }); + }); +}); diff --git a/src/core/reducer.ts b/src/core/reducer.ts index b10ae54f1..beba12b13 100644 --- a/src/core/reducer.ts +++ b/src/core/reducer.ts @@ -11,16 +11,23 @@ import * as plugins from '../plugins/main'; import { ProcessGameConfig } from './game'; import { error } from './logger'; import { INVALID_MOVE } from './constants'; +import type { Dispatch } from 'redux'; import type { ActionShape, Ctx, + ErrorType, Game, LogEntry, - State, - Move, LongFormMove, + Move, + State, + Store, + TransientMetadata, + TransientState, Undo, } from '../types'; +import { stripTransients } from './action-creators'; +import { ActionErrorType, UpdateErrorType } from './errors'; import { applyPatch } from 'rfc6902'; /** @@ -97,7 +104,7 @@ function initializeDeltalog( state: State, action: ActionShape.MakeMove | ActionShape.Undo | ActionShape.Redo, move?: Move -): State { +): TransientState { // Create a log entry for this action. const logEntry: LogEntry = { action, @@ -121,6 +128,79 @@ function initializeDeltalog( }; } +/** + * ExtractTransientsFromState + * + * Split out transients from the a TransientState + */ +function ExtractTransients( + transientState: TransientState | null +): [State | null, TransientMetadata | undefined] { + if (!transientState) { + // We preserve null for the state for legacy callers, but the transient + // field should be undefined if not present to be consistent with the + // code path below. + return [null, undefined]; + } + const { transients, ...state } = transientState; + return [state as State, transients as TransientMetadata]; +} + +/** + * WithError + * + * Augment a State instance with transient error information. + */ +function WithError( + state: State, + errorType: ErrorType, + payload?: PT +): TransientState { + const error = { + type: errorType, + payload, + }; + return { + ...state, + transients: { + error, + }, + }; +} + +/** + * Middleware for processing TransientState associated with the reducer + * returned by CreateGameReducer. + * This should pretty much be used everywhere you want realistic state + * transitions and error handling. + */ +export const TransientHandlingMiddleware = (store: Store) => ( + next: Dispatch +) => (action: ActionShape.Any) => { + const result = next(action); + switch (action.type) { + case Actions.STRIP_TRANSIENTS: { + return result; + } + default: { + const [, transients] = ExtractTransients(store.getState()); + if (typeof transients !== 'undefined') { + store.dispatch(stripTransients()); + // Dev Note: If parent middleware needs to correlate the spawned + // StripTransients action to the triggering action, instrument here. + // + // This is a bit tricky; for more details, see: + // https://github.com/boardgameio/boardgame.io/pull/940#discussion_r636200648 + return { + ...result, + transients, + }; + } + return result; + } + } +}; + /** * CreateGameReducer * @@ -142,8 +222,18 @@ export function CreateGameReducer({ * @param {object} state - The state before the action. * @param {object} action - A Redux action. */ - return (state: State | null = null, action: ActionShape.Any): State => { + return ( + stateWithTransients: TransientState | null = null, + action: ActionShape.Any + ): TransientState => { + let [state /*, transients */] = ExtractTransients(stateWithTransients); switch (action.type) { + case Actions.STRIP_TRANSIENTS: { + // This action indicates that transient metadata in the state has been + // consumed and should now be stripped from the state.. + return state; + } + case Actions.GAME_EVENT: { state = { ...state, deltalog: [] }; @@ -158,7 +248,7 @@ export function CreateGameReducer({ // Disallow events once the game is over. if (state.ctx.gameover !== undefined) { error(`cannot call event after game end`); - return state; + return WithError(state, ActionErrorType.GameOver); } // Ignore the event if the player isn't active. @@ -167,7 +257,7 @@ export function CreateGameReducer({ !game.flow.isPlayerActive(state.G, state.ctx, action.payload.playerID) ) { error(`disallowed event: ${action.payload.type}`); - return state; + return WithError(state, ActionErrorType.InactivePlayer); } // Execute plugins. @@ -200,7 +290,7 @@ export function CreateGameReducer({ ); if (move === null) { error(`disallowed move: ${action.payload.type}`); - return state; + return WithError(state, ActionErrorType.UnavailableMove); } // Don't run move on client if move says so. @@ -211,7 +301,7 @@ export function CreateGameReducer({ // Disallow moves once the game is over. if (state.ctx.gameover !== undefined) { error(`cannot make move after game end`); - return state; + return WithError(state, ActionErrorType.GameOver); } // Ignore the move if the player isn't active. @@ -220,7 +310,7 @@ export function CreateGameReducer({ !game.flow.isPlayerActive(state.G, state.ctx, action.payload.playerID) ) { error(`disallowed move: ${action.payload.type}`); - return state; + return WithError(state, ActionErrorType.InactivePlayer); } // Execute plugins. @@ -238,7 +328,8 @@ export function CreateGameReducer({ error( `invalid move: ${action.payload.type} args: ${action.payload.args}` ); - return state; + // TODO(#723): Marshal a nice error payload with the processed move. + return WithError(state, ActionErrorType.InvalidMove); } const newState = { ...state, G }; @@ -294,13 +385,14 @@ export function CreateGameReducer({ if (game.disableUndo) { error('Undo is not enabled'); - return state; + return WithError(state, ActionErrorType.ActionDisabled); } const { _undo, _redo } = state; if (_undo.length < 2) { - return state; + error(`No moves to undo`); + return WithError(state, ActionErrorType.ActionInvalid); } const last = _undo[_undo.length - 1]; @@ -311,7 +403,8 @@ export function CreateGameReducer({ actionHasPlayerID(action) && action.payload.playerID !== last.playerID ) { - return state; + error(`Cannot undo other players' moves`); + return WithError(state, ActionErrorType.ActionInvalid); } // If undoing a move, check it is undoable. @@ -322,7 +415,8 @@ export function CreateGameReducer({ last.playerID ); if (!CanUndoMove(state.G, state.ctx, lastMove)) { - return state; + error(`Move cannot be undone`); + return WithError(state, ActionErrorType.ActionInvalid); } } @@ -344,13 +438,14 @@ export function CreateGameReducer({ if (game.disableUndo) { error('Redo is not enabled'); - return state; + return WithError(state, ActionErrorType.ActionDisabled); } const { _undo, _redo } = state; if (_redo.length == 0) { - return state; + error(`No moves to redo`); + return WithError(state, ActionErrorType.ActionInvalid); } const first = _redo[0]; @@ -360,7 +455,8 @@ export function CreateGameReducer({ actionHasPlayerID(action) && action.payload.playerID !== first.playerID ) { - return state; + error(`Cannot redo other players' moves`); + return WithError(state, ActionErrorType.ActionInvalid); } state = initializeDeltalog(state, action); @@ -377,6 +473,7 @@ export function CreateGameReducer({ } case Actions.PLUGIN: { + // TODO(#723): Expose error semantics to plugin processing. return plugins.ProcessAction(state, action, { game }); } @@ -387,7 +484,7 @@ export function CreateGameReducer({ const hasError = patchError.some((entry) => entry !== null); if (hasError) { error(`Patch ${JSON.stringify(action.patch)} apply failed`); - return oldState; + return WithError(oldState, UpdateErrorType.PatchFailed, patchError); } else { return newState; } diff --git a/src/master/master.test.ts b/src/master/master.test.ts index 6d809519b..fb2021bdf 100644 --- a/src/master/master.test.ts +++ b/src/master/master.test.ts @@ -16,6 +16,7 @@ import { Auth } from '../server/auth'; import * as StorageAPI from '../server/db/base'; import * as dateMock from 'jest-date-mock'; import { PlayerView } from '../core/player-view'; +import { INVALID_MOVE } from '../core/constants'; jest.mock('../core/logger', () => ({ info: jest.fn(), @@ -83,6 +84,12 @@ function TransportAPI(send = jest.fn(), sendAll = jest.fn()) { return { send, sendAll }; } +function validateNotTransientState(state: any) { + expect(state).toEqual( + expect.not.objectContaining({ transients: expect.anything() }) + ); +} + describe('sync', () => { const send = jest.fn(); const db = new InMemory(); @@ -170,15 +177,19 @@ describe('update', () => { const sendAll = jest.fn((arg) => { sendAllReturn = arg; }); - const db = new InMemory(); - const master = new Master(game, db, TransportAPI(send, sendAll)); + const game = { + moves: { + A: (G) => G, + }, + }; + let db; + let master; const action = ActionCreators.gameEvent('endTurn'); - beforeAll(async () => { + beforeEach(async () => { + db = new InMemory(); + master = new Master(game, db, TransportAPI(send, sendAll)); await master.onSync('matchID', '0', undefined, 2); - }); - - beforeEach(() => { sendAllReturn = undefined; jest.clearAllMocks(); }); @@ -223,7 +234,7 @@ describe('update', () => { }); test('invalid matchID', async () => { - await master.onUpdate(action, 1, 'default:unknown', '1'); + await master.onUpdate(action, 0, 'default:unknown', '1'); expect(sendAll).not.toHaveBeenCalled(); expect(error).toHaveBeenCalledWith( `game not found, matchID=[default:unknown]` @@ -231,15 +242,15 @@ describe('update', () => { }); test('invalid stateID', async () => { - await master.onUpdate(action, 100, 'matchID', '1'); + await master.onUpdate(action, 100, 'matchID', '0'); expect(sendAll).not.toHaveBeenCalled(); expect(error).toHaveBeenCalledWith( - `invalid stateID, was=[100], expected=[1] - playerID=[1] - action[endTurn]` + `invalid stateID, was=[100], expected=[0] - playerID=[0] - action[endTurn]` ); }); test('invalid playerID', async () => { - await master.onUpdate(action, 1, 'matchID', '100'); + await master.onUpdate(action, 0, 'matchID', '100'); await master.onUpdate(ActionCreators.makeMove('move'), 1, 'matchID', '100'); expect(sendAll).not.toHaveBeenCalled(); expect(error).toHaveBeenCalledWith( @@ -248,15 +259,15 @@ describe('update', () => { }); test('invalid move', async () => { - await master.onUpdate(ActionCreators.makeMove('move'), 1, 'matchID', '1'); + await master.onUpdate(ActionCreators.makeMove('move'), 0, 'matchID', '0'); expect(sendAll).not.toHaveBeenCalled(); expect(error).toHaveBeenCalledWith( - `move not processed - canPlayerMakeMove=false - playerID=[1] - action[move]` + `move not processed - canPlayerMakeMove=false - playerID=[0] - action[move]` ); }); test('valid matchID / stateID / playerID', async () => { - await master.onUpdate(action, 1, 'matchID', '1'); + await master.onUpdate(action, 0, 'matchID', '0'); expect(sendAll).toHaveBeenCalled(); }); @@ -354,8 +365,15 @@ describe('update', () => { describe('undo / redo', () => { test('player 0 can undo', async () => { + const move = ActionCreators.makeMove('A', null, '0'); + await master.onUpdate(move, 0, 'matchID', '0'); + expect(error).not.toHaveBeenCalled(); + await master.onUpdate(ActionCreators.undo(), 1, 'matchID', '0'); + expect(error).not.toHaveBeenCalled(); + + // Negative case: All moves already undone. await master.onUpdate(ActionCreators.undo(), 2, 'matchID', '0'); - expect(error).not.toBeCalled(); + expect(error).toHaveBeenCalledWith(`No moves to undo`); }); test('player 1 can’t undo', async () => { @@ -371,30 +389,34 @@ describe('update', () => { [{ all: 'A' }], '0' ); - await master.onUpdate(setActivePlayers, 2, 'matchID', '0'); - await master.onUpdate(ActionCreators.undo('0'), 3, 'matchID', '0'); + await master.onUpdate(setActivePlayers, 0, 'matchID', '0'); + await master.onUpdate(ActionCreators.undo('0'), 1, 'matchID', '0'); expect(error).toHaveBeenCalledWith( `playerID=[0] cannot undo / redo right now` ); }); test('player can undo if they are the only active player', async () => { + const move = ActionCreators.makeMove('A', null, '0'); + await master.onUpdate(move, 0, 'matchID', '0'); + expect(error).not.toHaveBeenCalled(); const endStage = ActionCreators.gameEvent('endStage', undefined, '0'); - await master.onUpdate(endStage, 3, 'matchID', '0'); - await master.onUpdate(ActionCreators.undo('1'), 4, 'matchID', '1'); + await master.onUpdate(endStage, 1, 'matchID', '0'); + expect(error).not.toBeCalled(); + await master.onUpdate(ActionCreators.undo(), 2, 'matchID', '0'); expect(error).not.toBeCalled(); // Clean-up active players. const endStage2 = ActionCreators.gameEvent('endStage', undefined, '1'); - await master.onUpdate(endStage2, 4, 'matchID', '1'); + await master.onUpdate(endStage2, 3, 'matchID', '1'); }); }); test('game over', async () => { let event = ActionCreators.gameEvent('endGame'); - await master.onUpdate(event, 5, 'matchID', '0'); + await master.onUpdate(event, 0, 'matchID', '0'); event = ActionCreators.gameEvent('endTurn'); - await master.onUpdate(event, 6, 'matchID', '0'); + await master.onUpdate(event, 1, 'matchID', '0'); expect(error).toHaveBeenCalledWith( `game over - matchID=[matchID] - playerID=[0] - action[endTurn]` ); @@ -533,6 +555,9 @@ describe('patch', () => { stages: { A: { moves: { + Invalid: () => { + return INVALID_MOVE; + }, A: { client: false, move: (G, ctx: Ctx) => { @@ -560,6 +585,9 @@ describe('patch', () => { const action = ActionCreators.gameEvent('endTurn'); beforeAll(async () => { + master.subscribe(({ state }) => { + validateNotTransientState(state); + }); await master.onSync('matchID', '0', undefined, 2); }); @@ -624,7 +652,7 @@ describe('patch', () => { ); }); - test('invalid move', async () => { + test('disallowed move', async () => { await master.onUpdate(ActionCreators.makeMove('move'), 1, 'matchID', '0'); expect(sendAll).not.toHaveBeenCalled(); expect(error).toHaveBeenCalledWith( @@ -632,6 +660,17 @@ describe('patch', () => { ); }); + test('invalid move', async () => { + await master.onUpdate( + ActionCreators.makeMove('Invalid', null, '0'), + 1, + 'matchID', + '0' + ); + expect(sendAll).toHaveBeenCalled(); + expect(error).toHaveBeenCalledWith('invalid move: Invalid args: null'); + }); + test('valid matchID / stateID / playerID', async () => { await master.onUpdate(action, 1, 'matchID', '0'); expect(sendAll).toHaveBeenCalled(); @@ -640,7 +679,8 @@ describe('patch', () => { describe('undo / redo', () => { test('player 0 can undo', async () => { await master.onUpdate(ActionCreators.undo(), 2, 'matchID', '1'); - expect(error).not.toBeCalled(); + // The master allows this, but the reducer does not. + expect(error).toHaveBeenCalledWith(`No moves to undo`); }); test('player 1 can’t undo', async () => { @@ -667,7 +707,8 @@ describe('patch', () => { const endStage = ActionCreators.gameEvent('endStage', undefined, '1'); await master.onUpdate(endStage, 2, 'matchID', '1'); await master.onUpdate(ActionCreators.undo('0'), 3, 'matchID', '1'); - expect(error).not.toBeCalled(); + // The master allows this, but the reducer does not. + expect(error).toHaveBeenCalledWith(`Cannot undo other players' moves`); // Clean-up active players. const endStage2 = ActionCreators.gameEvent('endStage', undefined, '1'); @@ -720,6 +761,9 @@ describe('connectionChange', () => { beforeEach(() => { sendAllReturn = undefined; + master.subscribe(({ state }) => { + validateNotTransientState(state); + }); jest.clearAllMocks(); }); diff --git a/src/master/master.ts b/src/master/master.ts index 4a0534fe6..ddb5960f2 100644 --- a/src/master/master.ts +++ b/src/master/master.ts @@ -6,10 +6,13 @@ * https://opensource.org/licenses/MIT. */ -import { CreateGameReducer } from '../core/reducer'; +import { + CreateGameReducer, + TransientHandlingMiddleware, +} from '../core/reducer'; import { ProcessGameConfig, IsLongFormMove } from '../core/game'; import { UNDO, REDO, MAKE_MOVE } from '../core/action-types'; -import { createStore } from 'redux'; +import { createStore, applyMiddleware } from 'redux'; import * as logging from '../core/logger'; import { PlayerView } from '../plugins/main'; import type { @@ -202,7 +205,8 @@ export class Master { const reducer = CreateGameReducer({ game: this.game, }); - const store = createStore(reducer, state); + const middleware = applyMiddleware(TransientHandlingMiddleware); + const store = createStore(reducer, state, middleware); // Only allow UNDO / REDO if there is exactly one player // that can make moves right now and the person doing the diff --git a/src/plugins/main.ts b/src/plugins/main.ts index ddc3678d8..23bcc967f 100644 --- a/src/plugins/main.ts +++ b/src/plugins/main.ts @@ -41,6 +41,7 @@ export const ProcessAction = ( action: ActionShape.Plugin, opts: PluginOpts ): State => { + // TODO(#723): Extend error handling to plugins. opts.game.plugins .filter((plugin) => plugin.action !== undefined) .filter((plugin) => plugin.name === action.payload.type) diff --git a/src/types.ts b/src/types.ts index b0e37da93..7c0b306e5 100644 --- a/src/types.ts +++ b/src/types.ts @@ -2,6 +2,7 @@ import type { Object } from 'ts-toolbelt'; import type Koa from 'koa'; import type { Store as ReduxStore } from 'redux'; import type * as ActionCreators from './core/action-creators'; +import type { ActionErrorType, UpdateErrorType } from './core/errors'; import type { Flow } from './core/flow'; import type { CreateGameReducer } from './core/reducer'; import type { INVALID_MOVE } from './core/constants'; @@ -15,6 +16,7 @@ export type { StorageAPI }; export type AnyFn = (...args: any[]) => any; +// "Public" state to be communicated to clients. export interface State { G: G; ctx: Ctx | CtxWithPlugins; @@ -27,6 +29,30 @@ export interface State { _stateID: number; } +export type ErrorType = UpdateErrorType | ActionErrorType; + +export interface ActionError { + type: ErrorType; + // TODO(#723): Figure out if we want to strongly type payloads. + payload?: any; +} + +export interface TransientMetadata { + error?: ActionError; +} + +// TODO(#732): Actually define a schema for the action dispatch results. +export type ActionResult = any; + +// "Private" state that may include garbage that should be stripped before +// being handed back to a client. +export interface TransientState< + G extends any = any, + CtxWithPlugins extends Ctx = Ctx +> extends State { + transients?: TransientMetadata; +} + export type PartialGameState = Pick; export type StageName = string; @@ -403,6 +429,9 @@ export namespace ActionShape { export type Reset = ReturnType; export type Undo = StripCredentials; export type Redo = StripCredentials; + // Private type used only for internal error processing. + // Included here to preserve type-checking of reducer inputs. + type _StripTransients = ReturnType; export type Any = | MakeMove | GameEvent @@ -413,7 +442,8 @@ export namespace ActionShape { | Reset | Undo | Redo - | Plugin; + | Plugin + | _StripTransients; } export namespace ActionPayload {