Adds check to throw error if non-serializable state is used in a move

[vdfdev]

This check is meant to be used only in unit-tests to avoid breaking games that might be working (by chance) in production.

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

[vdfdev]

Context:
This is a follow-up on this issue freeboardgames/FreeBoardGames.org#754
caused by a bad bug in our most popular game (checkers): freeboardgames/FreeBoardGames.org#750
caused by a well-meaning refactor that slipped through code review that started using classes which are not serializable, and broke only on online mode (and still worked in local mode)

[delucis]

Thanks for this @flamecoals! I think it’s a great idea — I’ve seen a few people trip up on this after prototyping locally and then moving to a server.

A couple of things:

• Not everyone writes tests, but it would be nice to provide this feedback to everyone, so I would suggest moving the check behind process.env.NODE_ENV !== 'production'. Bundlers should then strip it out of production builds, but the errors will be surfaced more quickly if someone starts trying to add classes, functions, Maps, Sets or DOM nodes (!!) to state.

• Using parse/stringify/deepStrictEqual seems to throw up a few false positive as you noted with state like { moved: undefined }. (While it’s true that that becomes {}, I would still expect that to pass the test as these are equivalent in practice.)

  I would suggest using a more explicit check like this:

  import isPlainObject from "lodash/isPlainObject";
  
  /**
   * Check if a value can be serialized (e.g. using `JSON.stringify`).
   * Adapted from: https://stackoverflow.com/a/30712764/3829557
   */
  function isSerializable(value: any) {
    // Primitives are OK.
    if (
      value === undefined ||
      value === null ||
      typeof value === 'boolean' ||
      typeof value === 'number' ||
      typeof value === 'string'
    ) {
      return true;
    }
  
    // A non-primitive value that is neither a POJO or an array cannot be serialized.
    if (!isPlainObject(value) && !Array.isArray(value)) {
      return false;
    }
  
    // Recurse entries if the value is an object or array.
    for (const key in value) {
      if (!isSerializable(value[key])) return false;
    }
  
    return true;
  };

What do you think?

[vdfdev]

Great! I agree with both points. I will patch the PR soon

[delucis]

@flamecoals Great, thanks!

We could also potentially add this logic as a new default plugin to avoid adding extra stuff to the core game function:

const CheckSerializablePlugin = {
  name: 'check-serializable',

  fnWrap: (fn) => (G, ctx, ...args) => {
    G = fn(G, ctx, ...args);
    if (!isSerializable(G)) throw new Error(/* ... */);
    return G;
  };
};

(The Immer plugin is probably a good reference point for what this might look like with types.)

[vdfdev]

I didn't forget about this PR, life has just been extra busy on the past few days (I was moving houses). I think I will find some time to work on this soon

[delucis]

@flamecoals Thanks for letting us know. No rush!

[vdfdev]

@delucis @nicolodavis sorry for the delay, I finally finished setting up everything in the new house.

I think I addressed all the points above, PTAL.

[delucis]

Looks good @flamecoals. I have one small suggestion to refactor the plugin code, but this looks fundamentally solid. Thanks!

[vdfdev]

Accepted suggestion

[delucis]

@flamecoals Thanks for this!