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

[shaoster]

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

Checklist

• Use a separate branch in your local repo (not master).
• Test coverage is 100% (or you have a story for why it's ok).

[delucis]

Thanks @shaoster! Good job separating out this chunk of the work. Here are a few initial comments.

Regarding the general architecture, it’s a little more complicated than reducer → master → client. If you search for CreateGameReducer in the repo, you’ll find all the places it’s used, but here’s a quick overview:

1. The reducer runs directly inside the client, this allows the client to run without a server/local master and allows the client to update optimistically even before receiving an update from the server when running a multiplayer game.

2. When in use, the reducer runs in the master either on the server or inside Local (this is the scenario you already considered).

3. AI code uses the reducer to run simulations. (The same is true in the MCTS bot implementation.)

4. Finally, we also currently run the reducer when stepping through items in the Log pane of the debug panel.

Implications

• In the client we need to consider the middleware added to the store to see how exactly to handle the transient state and later surface transients.error when produced by the local reducer. As elsewhere, adding transients doen’t cause any actual errors, but presumably we’d want to strip it as in the master and use the error somehow.

• 3 & 4 can perhaps wait — adding the transients field shouldn’t actually cause problems here. It may later be possible to use transients.error to improve the logic in AI simulations and log stepping (this latter could perhaps be done as part of Stepping through log outside of debug? #892)

[shaoster]

Thanks @shaoster! Good job separating out this chunk of the work. Here are a few initial comments.

Regarding the general architecture, it’s a little more complicated than reducer → master → client. If you search for CreateGameReducer in the repo, you’ll find all the places it’s used, but here’s a quick overview:

1. The reducer runs directly inside the client, this allows the client to run without a server/local master and allows the client to update optimistically even before receiving an update from the server when running a multiplayer game.
2. When in use, the reducer runs in the master either on the server or inside Local (this is the scenario you already considered).
3. AI code uses the reducer to run simulations. (The same is true in the MCTS bot implementation.)
4. Finally, we also currently run the reducer when stepping through items in the Log pane of the debug panel.

Implications

• In the client we need to consider the middleware added to the store to see how exactly to handle the transient state and later surface transients.error when produced by the local reducer. As elsewhere, adding transients doen’t cause any actual errors, but presumably we’d want to strip it as in the master and use the error somehow.
• 3 & 4 can perhaps wait — adding the transients field shouldn’t actually cause problems here. It may later be possible to use transients.error to improve the logic in AI simulations and log stepping (this latter could perhaps be done as part of Stepping through log outside of debug? #892)

The latest version has dealt with 1 & 2 as quoted above.

My implementation strategy of creating a new middleware, shared between the client and master, suggests that a more fundamental unification of those code paths might be beneficial at some point. Having some kind of shared component that deals with all dispatches (and perhaps adding client/master specializations in an outer-most middleware) could be easier to maintain.

Don't want to bite off more than I can chew though!

[delucis]

Hey @shaoster, here’s a new review. I’m a little confused about what the new middleware is trying to do 😅

---

Edit: Did more reading, checked the code again, and I understand a little better. Here’s where my understanding is currently (correct me if I’m wrong):

• TransientHandlingMiddleware intercepts any transients the reducer produces, strips them from the store and then returns them at the dispatch call site alongside the original action object. Example return:

  {
    type: 'MAKE_MOVE',
    payload: { ... },
    stripTransientsResult: { type: 'STRIP_TRANSIENTS' }, // invariant so perhaps superfluous?
    transients: { error: { type: 'action/invalid_move', payload: undefined } },
  }

Automatically stripping the transients seems like a good idea. I’m wondering if there’s a better way to surface the error than returning it though.

Would it make sense to instead follow a model more like that of the SubscriptionMiddleware in the client? I mean to say that basically a transient handler could strip transients and then trigger side effects, rather than returning the transients. Something like:

const createErrorHandlingMiddleware =
  (onError: (error: ActionError) => void) =>
  (store: Store) =>
  (next: Dispatch<ActionShape.Any>) =>
  (action: ActionShape.Any) => {
    const result = next(action);
    const [, transients] = ExtractTransients(store.getState());
    if (typeof transients !== 'undefined') {
      store.dispatch(stripTransients());
      if (transients.error) onError(transients.error);
    }
    return result;
  };

// Usage
const middleware = applyMiddleware(
  createErrorHandlingMiddleware((error) => {
    // side effects
  })
);

I’m not sure if that’s quite what we want, but basically I think we’re missing one more step to wire up the transients in the master and the client so I can get my brain around where this is going, so I’m curious what you’re planning.

Oudated due to new context/understanding

[delucis]

Thanks for the extra context — the piece I was missing was the eventual intention of how the transients attached to the action object would be consumed.

I’ve made a few suggestions to tidy up and then I guess the final thing needed for this PR would be to go through implementing the errors that haven’t yet been assigned an error code in the reducer, right?

[shaoster]

Review feedback addressed. PTAL

[delucis]

Ok, I think we’re getting there! One small fix and a question.

[delucis]

Do you need a hand deciding on names for the various commented out errors here and below or do you want to have a go at creating error types for these? Or are you reserving this for a separate PR?

[shaoster]

I'm fine with either:

1. Taking some help to decide on error types and updating the tests as part of this change.
2. Just leaving this as a TODO to the next PR that starts adding client semantics.

[delucis]

I think it would be nice to include these as part of this PR so we have the reducer code basically done. I’ll take a look and make some suggestions.

[delucis]

OK, so the cases to cover are:

• No moves to undo
• Cannot undo other players' moves
• Move cannot be undone
• No moves to redo
• Cannot redo other players' moves

Do you think collapsing these all into a single error type would be OK?

• action/action_invalid — The requested action is not currently possible

It would make handling each specific case separately, but keeping the total number of error types down also seems desirable.

[shaoster]

So when trying to update the master tests after raising these errors, I found several tests that don't do what they claim to be doing and, in general, that file looks like it needs to be reworked. There's a ton of dependence between different test cases AND different test suites, and they are extremely order dependent.

It's also super unclear what, if any, coverage the seemingly duplicated tests are adding compared to a parameterized test on the game.deltaState field.

Thus, I'm going to do the minimal thing to get these tests passing and covering the updated behavior.

Update

It looks like there's one genuine bug that the test fixes are uncovering:

1. Fresh game state. No moves, and 2 players.
2. Call events.endStage(). This adds an undo entry to the _undo log, but this isn't a move (i.e. no moveType).
3. Try to undo. This fails on:

   TypeError: Cannot read property 'undoable' of null
   
     47 | const CanUndoMove = (G: any, ctx: Ctx, move: Move): boolean => {
     48 |   function HasUndoable(move: Move): move is LongFormMove {
   > 49 |     return (move as LongFormMove).undoable !== undefined;
        |                                   ^
     50 |   }
     51 | 
     52 |   function IsFunction(
   
     at HasUndoable (src/core/reducer.ts:49:35)
     at CanUndoMove (src/core/reducer.ts:58:8)
     at src/core/reducer.ts:417:14
     at dispatch (node_modules/redux/lib/redux.js:218:22)
     at Object.dispatch (src/core/reducer.ts:180:18)
     at Master.onUpdate (src/master/master.ts:271:11)
     at Object.<anonymous> (src/master/master.test.ts:402:20)
   

The test that uncovered this: update > undo / redo > player can undo if they are the only active player (https://github.com/boardgameio/boardgame.io/blob/master/src/master/master.test.ts#L381) was previously passing (erroneously) because undoing the previous player's move didn't error and just silently continued (https://github.com/boardgameio/boardgame.io/blob/master/src/core/reducer.ts#L312).

See traces from master:

Action (Notice playerID == 1):

console.error internal/console/constructor.js:409
    Trace: { type: 'UNDO', payload: { type: null, args: null, playerID: '1' } }
        at /home/phil/code/boardgame-master/src/core/reducer.ts:309:17
        at Object.dispatch (/home/phil/code/boardgame-master/node_modules/redux/lib/redux.js:218:22)
        at Master.onUpdate (/home/phil/code/boardgame-master/src/master/master.ts:267:11)
        at Object.<anonymous> (/home/phil/code/boardgame-master/src/master/master.test.ts:384:20)
        at processTicksAndRejections (internal/process/task_queues.js:93:5)

Last (Notice playerID == 0):

console.error internal/console/constructor.js:409
    Trace: {
    G: {},
    ctx: {
        numPlayers: 2,
        turn: 3,
        currentPlayer: '0',
        playOrder: [ '0', '1' ],
        playOrderPos: 0,
        phase: null,
        activePlayers: { '1': 'A' },
        _activePlayersMoveLimit: null,
        _activePlayersNumMoves: { '0': 0, '1': 0 },
        _prevActivePlayers: [],
        _nextActivePlayers: null,
        numMoves: 0
    },
    plugins: {
        random: { data: [Object] },
        log: { data: {} },
        events: { data: {} }
    },
    playerID: '0'
    }
        at /home/phil/code/boardgame-master/src/core/reducer.ts:310:17
        at Object.dispatch (/home/phil/code/boardgame-master/node_modules/redux/lib/redux.js:218:22)
        at Master.onUpdate (/home/phil/code/boardgame-master/src/master/master.ts:267:11)
        at Object.<anonymous> (/home/phil/code/boardgame-master/src/master/master.test.ts:384:20)
        at processTicksAndRejections (internal/process/task_queues.js:93:5)

[shaoster]

Should I raise a bug for ^ and fix that first?

[delucis]

Good catch! A fix PR would be ideal (no need to open an issue for something like this).

[shaoster]

Opened #942 to fix this underlying issue.

[delucis]

@shaoster I’ve merged your fix PR. Let me know if you need a hand resolving the conflict with this branch.

[shaoster]

De-conflicted against #942 and local change for the additional error cases rebased on top.

[delucis]

Fantastic work! Particularly isolating all those test cases instead of having them depend on test order. I think this looks good to go 🎉