consider using Immer

[nicolodavis]

People get immutability wrong all the time (self included). Even the ones that know what they're doing often have to write some really messy code when deep state updates are involved.

Immer seems to achieve the best of both worlds. We should consider bundling it in (and possible have a flag that allows turning it off for people that prefer not to use it).

move: (G, ctx) {
  G.hand++;
  G.deck--;
}

vs.

move: (G, ctx) {
  return {
    ...G,
    hand: G.hand + 1,
    deck: G.deck - 1,
  };
}

Open questions:

1. How do we indicate invalid moves ( Illegal moves API #292 ) in this approach?

[philihp]

I would like to suggest the following syntax, as to avoid breaking existing moves, and is similar to normal Redux action creators vs. Redux-thunk action creators.

• If the move comes back with an object (or an undefined), nothing is different. Assume the move function properly follows immutability practices (a equality of the input to output is also a deep equality).
• If the move comes back with a function, then let's it's an producer, and pass that to immer's produce()

// default (current) behavior, process without Immer
drawCard: (G, ctx) => ({
  ...G,
  hand: G.hand + 1,
  deck: G.deck - 1,
})

// process with Immer
drawCard: (prevG, ctx) => nextG => {
  nextG.hand++;
  nextG.deck--;
}

In this manner, an invalid move returning undefined could be expressed as

drawCard: (prevG, ctx) => {
  if(prevG.deck === 0) return undefined
  return nextG => {
    nextG.hand++;
    nextG.deck--;
  }
}

[nicolodavis]

Immer allows you to return the new state, so we can just turn on Immer in a backwards compatible way without needing to return a thunk.

I think both the following would be valid:

drawCard: (G, ctx) => {
  G.hand++;
  G.deck--;
}

drawCard: (G, ctx) => ({
  ...G,
  hand: G.hand + 1,
  deck: G.deck - 1,
})

so people can just choose the style that they prefer.

The main problem is the invalid move API. One option would be to do something like this:

import { INVALID_MOVE } from 'boardgame.io/core';

drawCard: (G, ctx) => {
  if (G.deck === 0) return INVALID_MOVE;
  ...
}

I think it's reasonable to disallow setting G to an string / integer constant so that INVALID_MOVE is never confused with actual game state.

[philihp]

It feels like that's enabling people to continue to get immutability wrong. How about this syntax?

drawCard: (G, ctx) => {
  this.hand++;
  this.deck--;
}

Regarding INVALID_MOVE, that looks like a reasonable syntax. The immer nothing constant could be aliased, and we'd get that behavior basically for free.

import { nothing } from "immer"
export const INVALID_MOVE = nothing

[nicolodavis]

I prefer to think of it more as Immer allowing people not to think about immutability (rather than enabling them to get it wrong). For people that are not familiar with the concept, I think there is a basic expectation that they should be able to mutate G and everything should just work. Immer enables this while also being performant (and also providing an escape hatch for people that want to do immutability manually).

So I think we should just allow (Immer handled) mutations on Grather than using this or some other syntax, especially considering that we're not going to break existing moves (with the small caveat that we'll be changing the Illegal Moves API, which we might have done anyway in #292).

[philihp]

I can see the advantage of having current code errantly mutating state being safe once this is implemented. Immer automatically binds this to the draft of G inside of produce, so I would expect both should work.

[nicolodavis]

Supported in 0.28.0.