feat: Expose method types to plugin fnWrap & improve events errors

[delucis]

Here’s an attempt to close #979. Feedback very much wanted and if you want to pick this up and iterate on it, you’re welcome to as I likely won’t have much time in the next couple of weeks.

Also closes #981.

Approach

When wrapping game hooks (onBegin, onMove, onEnd), provide a unique string to the plugin interface so that it knows which hook is being wrapped. (These unique strings are from a kind of custom enum accessed like GameMethod.Phase.ON_BEGIN or GameMethod.MOVE.)

A plugin’s fnWrap then receives this string in addition to the function to wrap, so it can change behaviour appropriately.

const plugin = {
  name: 'discerning-wrapper',
  fnWrap: (fn, fnType) => (...args) => {
    if (fnType === GameMethod.Turn.ON_MOVE) {
      console.log('Looks like this is an onMove hook.);
    }
    return fn(...args);
  },
};

In the events plugin we store this string, like we do with the turn/phase details and use it when processing events. This is actually pretty nice because we can give more informative errors. The main drawback is that this depends on fnWrap which is not applied to triggers (like endIf) and other game methods (like turn.order.first), so the best we can do is error if the hook type is undefined (which is probably OK, because only hooks should be used for events) without knowing exactly what method triggered the error.

Notes

I don’t particularly like that this exposes something new to the public plugin API, but I couldn’t really see how to do this otherwise as that is how the events plugin is built. One alternative could be to only expose this new data via some separate plugin field, like _internal_fnWrap, which would still be public but marked as private. Not sure exactly what the best approach is, but happy to see alternative approaches.

To-do

• If we go with this model, we should expose the GameMethod object properly, so that plugin authors can import it. Probably from the boardgame.io/core package?
• Are there any other scenarios where it would be good to error with some useful diagnostic information?

---

CC: @cristiands7 — Happy to hear your thoughts too!

[cristiands7]

Another option could be:

const plugin = {
  name: 'discerning-wrapper',
  fnWrap: (fn) => (...args) => {
    if (fn._fnType === GameMethod.Turn.ON_MOVE) {
      console.log('Looks like this is an onMove hook.);
    }
    return fn(...args);
  },
};

[vdfdev]

Can we change from a string to an enum, in order to gain type safety?

[delucis]

Can we change from a string to an enum, in order to gain type safety?

Sure. I liked the expressiveness of the enum-like object (GameMethod.Phase.ON_BEGIN vs GameMethod.PHASE_ON_BEGIN), but I guess using a genuine enum would enforce importing the GameMethod for Typescript users.

What do you think about @cristiands7’s suggestion? I like that it’s a more “private” approach, but am a little wary of this kind of Fn & { _fnType: GameMethod } type.

[delucis]

Here’s a summary of the scenarios that now produce errors. Some of these were previously harmless/ignored silently, so may produce breaks in some games, but hopefully this extra diagnostic information will steer people to better understanding the game flow.

• Event called outside hook

  Events must be called from moves or the onBegin, onEnd, and onMove hooks. This error probably means you called an event from other game code, like an endIf trigger or one of the turn.order methods.

• endTurn called in onEnd

  endTurn is disallowed in onEnd hooks — the turn is already ending. (Previously ignored silently.)

• Maximum number of turn endings exceeded

  This likely means game code is triggering an infinite loop.

• Phase event in phase.onEnd

  setPhase & endPhase are disallowed in a phase’s onEnd hook — the phase is already ending. If you’re trying to dynamically choose the next phase when a phase ends, use the phase’s next trigger. (Previously ignored silently.)

• Stage event in onEnd

  setStage, endStage & setActivePlayers are disallowed in onEnd hooks. (Previously ignored silently.)

• Stage event in phase.onBegin

  setStage, endStage & setActivePlayers are disallowed in a phase’s onBegin hook. Use setActivePlayers in a turn.onBegin hook or declare stages with turn.activePlayers instead. (Previously ignored silently.)

• Stage event in turn.onBegin

  setStage & endStage are disallowed in turn.onBegin. Use setActivePlayers or declare stages with turn.activePlayers instead. (Previously permitted but buggy, closes setStage in turn.onBegin always uses playerID 0 #981.)

Which hooks support which events

	turn onMove	turn onBegin	turn onEnd	phase onBegin	phase onEnd	game onEnd
setStage	✅	❌	❌	❌	❌	❌
endStage	✅	❌	❌	❌	❌	❌
setActivePlayers	✅	✅	❌	❌	❌	❌
endTurn	✅	✅	❌	✅	❌	❌
setPhase	✅	✅	✅	✅	❌	❌
endPhase	✅	✅	✅	✅	❌	❌
endGame	✅	✅	✅	✅	✅	❌

✅ = supported     ❌ = not supported

(The state does not actually error in the game.onEnd case — hopefully it is obvious enough that you can’t change turn etc. at the end of the game and I didn’t want to break people’s games right before they end when no actual harm can be done by events.)