[Bugfix] Events should be undoable

[shaoster]

See #940 (comment)

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 for tackling this. I don’t think this is actually the logic we want. It should be possible to undo just the endStage event in your test and not the event and the move. (An endStage called by a move via the ctx.events API won’t end up in the undo/redo stack, so you only have to account for the case where an event was explicitly called.)

My suggestion is to wrap this reducer check in if (last.moveType):

boardgame.io/src/core/reducer.ts

Lines 317 to 325 in dc6cd45

	// Only allow undoable moves to be undone.
	const lastMove: Move = game.flow.getMove(
	restore.ctx,
	last.moveType,
	last.playerID
	);
	if (!CanUndoMove(state.G, state.ctx, lastMove)) {
	return state;
	}

i.e. we only check for move undoability if the thing to be undone is actually a move.

The undo/redo stacks are reset at the start of each turn/phase, so this only enables event undos for stages anyway. (And we already made sure we’re not undoing another player’s action, so this only allows a player to undo their own stage event.)

[shaoster]

Thanks for tackling this. I don’t think this is actually the logic we want. It should be possible to undo just the endStage event in your test and not the event and the move. (An endStage called by a move via the ctx.events API won’t end up in the undo/redo stack, so you only have to account for the case where an event was explicitly called.)

My suggestion is to wrap this reducer check in if (last.moveType):

boardgame.io/src/core/reducer.ts

Lines 317 to 325 in dc6cd45

	// Only allow undoable moves to be undone.
	const lastMove: Move = game.flow.getMove(
	restore.ctx,
	last.moveType,
	last.playerID
	);
	if (!CanUndoMove(state.G, state.ctx, lastMove)) {
	return state;
	}

i.e. we only check for move undoability if the thing to be undone is actually a move.

The undo/redo stacks are reset at the start of each turn/phase, so this only enables event undos for stages anyway. (And we already made sure we’re not undoing another player’s action, so this only allows a player to undo their own stage event.)

The main reason I didn't want to go the route of actually supporting undo of endStage is that changes existing behavior and smells more like a feature than a bugfix.

Remember the cause for tackling this was this stack trace where the last thing on the stack wasn't a move:

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)

[delucis]

Well, whether it’s a feature or a bug fix, I still think it’s what we want :-D

I guess it’s just an argument as to whether the bug was the thing without a moveType ending up on the undo stack, or the code processing the undo stack failing to handle a thing without a moveType. I’d suggest the latter but 🤷

[shaoster]

Well, whether it’s a feature or a bug fix, I still think it’s what we want :-D

I guess it’s just an argument as to whether the bug was the thing without a moveType ending up on the undo stack, or the code processing the undo stack failing to handle a thing without a moveType. I’d suggest the latter but 🤷

SGTM about that being the more reasonable behavior. Main concern is that it's bigger scope to test that "new" functionality because of the current test setup. I guess I can fix the tests accordingly to cover! 🚀

RE: i.e. we only check for move undoability if the thing to be undone is actually a move.: shouldn't we have exactly the same pre-conditions for undoing a move vs. an event? Edit: nvm stages don't seem to have that restriction

[delucis]

Main concern is that it's bigger scope to test that "new" functionality because of the current test setup. I guess I can fix the tests accordingly to covera! 🚀

Thanks! I guess it shouldn’t be too different from the existing tests, just with the endStage undoing as expected.

[delucis]

Looks good @shaoster! Thank you.