boardgameio / boardgame.io

State Management and Multiplayer Networking for Turn-Based Games
https://boardgame.io
MIT License
9.99k stars 704 forks source link

Discard changes to G if a move is invalid #687

Closed MathieuLoutre closed 4 years ago

MathieuLoutre commented 4 years ago

Immer is super handy when making complex changes in a move rather than using immutable update patterns in a deep nested object. However, I have a few occasions when I'd like to discard some changes I've made if it turns out the move is invalid.

For instance, when a card is played in a particular way, a corresponding token is put on the board in a starting space and then is moved to another space on the board. It's easier to put the token on the board and then check if the move is valid so I modify G and run the check, but if it's invalid and I return INVALID_MOVE, Immer rightfully complains.

I tried messing with Immer's original function but it doesn't seem to do what I want. Ideally, before I return INVALID_MOVE I'd like to make as if I hadn't modified G at all and reset it to its previous state. That sounds like something that should be possible but I can't figure out how.

nicolodavis commented 4 years ago

I haven't found a good solution to this either. Does making the changes in a copy of G work? You can use that to check if the move is invalid and then copy it over the the original if you want the move to proceed.

MathieuLoutre commented 4 years ago

I did try to make a copy of G like so const draftG = { ...G } and making the changes to draftG but I still get an error. Could be I'm doing something wrong somewhere else, but my impression was that the underlying objects are still being proxied by Immer. Might be worth doing a reduced test case?

Could also be something to bring up with the folks over at Immer? It might not fit the use cases they want to cover since you can already return NOTHING if you want to discard the changes (if I got that correctly). Is INVALID_MOVE a wrapper around Immer's NOTHING?

delucis commented 4 years ago

I think Immer’s nothing is used if you want to reset your state to undefined (which returning undefined won’t do, nor assigning state = undefined) (docs).

Could the problem with const draftG = { ...G } be nested object references? It’s more expensive but maybe some kind of deep copying could work or do proxies interfere with that? Something as simple as const draftG = JSON.parse(JSON.stringify(G))?

I’m not very familiar with Immer, but just reading through the docs, I see they mention a function for getting the draft’s original state.

Could we use that to do something like the following internally?

import { original, produce } from 'immer';

const newG = produce(oldG, G => {
  const moveResult = moveFn(G, ctx);
  if (moveResult === INVALID_MOVE) return original(G);
  return moveResult;
});

This would be great, because INVALID_MOVE could be returned at any point in move functions instead of validation all having to happen before any mutations.

MathieuLoutre commented 4 years ago

@delucis definitely something to do with the nesting for the draftG idea. Might be worth a try but I think I like your suggested approach better. As you say, you can return INVALID_MOVE at any point and that's very handy.

I did try their original function but inside the move but no luck. Doing it as you suggest would solve that issue brilliantly!

delucis commented 4 years ago

I did try their original function but inside the move but no luck.

Well, it’s also very possible I’ve misunderstood what original does 😅

If it is theoretically sound, we would have to update the immer plugin and figure out a way to flag to outside code that the move was invalid. Currently the reducer checks the return to bail out of events etc.:

https://github.com/nicolodavis/boardgame.io/blob/2f74b548b53b69ee028cfd73d50032e36ff16b64/src/core/reducer.ts#L166-L172