feat: deprecate moveLimit in favour of minMoves/maxMoves

[senritsu]

• EndTurn now checks minMoves instead of moveLimit like before to determine if ending the turn is possible
• EndStage now checks minMoves similar to how EndTurn does, previously it seemed to have no such check; in contrast to EndTurn it does not use the phase config, but instead checks the resolved values directly to support usage of minMoves in the args of setActivePlayers
• moveLimits is kept as-is, to prevent breaking changes; a rename to maxMoves or introducing maxMoves as an alias while deprecating moveLimits may be done at a later stage

In most places, I tried to keep the usage as much in-line with moveLimits as possible. Notable exceptions:

• moveLimits is used for checking if a turn should be ended automatically (where minMoves is not)
• minMoves is used to determine if manually ending a turn or stage is possible (where moveLimits now has no relevance anymore).

As this is my first time doing a deep-dive into the codebase, please give a thorough sanity check to everything. I am especially unsure about the integration in EndStage, and possible repercussions of checking or not checking for minMoves there. Additionally I am not sure if there are more places where a check would be needed.

I also noticed that Process in flow.ts does not check if it should auto-EndStage based on moveLimits, in contrast to the similar check for EndTurn. Is this on purpose?

Checklist

• Use a separate branch in your local repo (not main).
• Test coverage is 100%

---

Closes #934

[delucis]

@senritsu Thanks for this! I’ll try to get a review to you soon.

[senritsu]

Thinking about this some more, I am not sure if the whole minMoves/maxMoves (currently moveLimit) split in it's current incarnation is worth the complexity it adds for its fairly constrained use.

From how I understand it, moveLimit is really just a convenience option for

endIf: (G, ctx) => ctx.somewhere.numMoves >= MOVE_LIMIT

On the other hand, minMoves is really only useful for one thing, blocking a manual endTurn (or endStage). This would probably be better served by a similar predicate to endIf, something like a hypothetical canEnd.

canEnd: (G, ctx) => ctx.somewhere.numMoves >= MIN_MOVES

This would have the additional benefit of being way more flexible and useful, since it can be used to check for arbitrarily complex constraints and not just the number of moves. Additionally it would also be very useful for the UI, since a manual "End turn" button could be disabled properly when the turn (or phase/stage) can't be ended.

This seems like a better tradeoff between complexity budget and functionality, and it also raises the question if moveLimit itself is really necessary. Seeing as the game config is not serializable anyways, it doesn't seem to offer much benefit over a lambda in endIf and does add a fair bit of really specialized code.

[senritsu]

To expand on the previous point, supposing the mentioned predicates both exist, it would also be fairly trivial to replace the convenience of a simple moveLimit property by a collection of higher-order functions. This could also expand the out-of-the-box functionality for some more obscure scenarios like all players having a shared move-pool.

endIf: afterMoves(4) // equivalent to old `moveLimit`
endIf: madeMoves(4) // expands to (G, ctx) => ctx.something.numMoves >= 4
endIf: moveLimit({currentPlayer: 2, others: 3}) // different limits per player
endIf: moveCount({allCombined: 5}) // shared limit for all players
// functions should work identical for both predicates, naming should be carefully chosen to fit both
canEnd: afterMoves(4)
canEnd: madeMoves(4)
canEnd: moveLimit({currentPlayer: 2, others: 3})
canEnd: moveCount({allCombined: 5})

As an additional bonus, the relation between the somewhat unclear moveLimit and endTurn would be clarified slightly since it is explicitely assigned to endIf.

[delucis]

Thanks for the thoughts — lot of good ideas there.

This proposal would also impact stages — currently stages have a moveLimit option that can be passed (see also the stage move limit docs):

setActivePlayers({ others: 'A', moveLimit: 2 });

Because this is dynamic, the move limit has to be serializable. (In general we’ve also steered clear of hooks for stages to avoid developers needing to think about who is in which stage too much — i.e. unlike turn.endIf, stage.endIf would be called per player rather than just once. Not impossible to understand obviously, but a declarative moveLimit has less moving parts.) There was a proposal (#973) to add a declarative moveLimit to stage configs themselves too, which seems a good idea, but endIf/canEnd here wouldn’t allow us to remove any of the moveLimit complexity because they couldn’t provide the dynamic moveLimit functionality.

Given that, for consistency across turns and stages, I think we can’t throw out support for the declarative approach. That said, I don’t see anything against a canEnd method for more flexibility.

In theory, we could refactor support for the moveLimit or maxMoves option to wrap endIf. At the top of flow.ts there’s a bunch of code that normalizes a game definition for use. We could convert things there something like:

    // Deprecate `moveLimit` without a hard break for now.
    if (phaseConfig.turn.moveLimit !== undefined) {
      logging.info(
        '`turn.moveLimit` is deprecated. Please use `turn.maxMoves` & `turn.minMoves` instead.'
      );
      phaseConfig.turn.minMoves = phaseConfig.turn.moveLimit;
      phaseConfig.turn.maxMoves = phaseConfig.turn.moveLimit;
    }

    // Wrap `endIf` to check the move limit.
    if (phaseConfig.turn.maxMoves !== undefined) {
      const { endIf, maxMoves } = phaseConfig.turn;
      phaseConfig.turn.endIf = (G, ctx) =>
        endIf(G, ctx) || ctx.numMoves >= maxMoves;
    }

Then code in the rest of the flow reducer only needs to worry about calling endIf, not specifically checking moveLimit/maxMoves. The same approach could be used with canEnd and a minMoves option.

What do you think?

---

(Just tried this and it works, but I think wrapping endIf is no better than the current situation where we check move limits in ShouldEnd — no reduction in complexity, just less clear intent.)

[senritsu]

Stages/Dynamic

I did actually wonder about how it would integrate with Stages, and I do agree that less cognitive overhead there is definitely a worthwhile goal.

Supporting the dynamic case also makes a lot of sense, generally I am a big fan of data-driven/declarative approaches. Professionally I currently use mapbox, which does a lot of cool things in that direction, but having full serializable expression support may be a bit overkill at the moment. Worth a look for inspiration though, if the declarative approach is something that should be a focus.

Intuitively it would kind of feel bad if the dynamic case were severely limited compared to the hook-based equivalent, but maybe in actual use it wouldn't matter too much. Having incomplete parallels between Turns and Stages feels similar.

Wrapping functions

Regarding implementing the serializable options with a hook wrapper, the main argument in favor would probably be extensibility and simplicity in terms of how much stuff needs to be carted around or inherited during the whole flow/context bookkeeping. There are roughly 35 usages of the internal lookup where the per-player move limits are stored, double now with minMoves.

If internally everything would be compiled down to hook wrappers (which would probably require at least internal support for Stage hooks, in which case you might as well make them public for advanced use cases), it might cut down quite a bit of code, while simultaneously opening the path to add any number of similar declarative options (and of course supporting completely arbitrary behavior via the hooks themselves). The full ramifications are hard to theory-craft though, I would probably have to try some things hands-on to get a clearer picture.

Conclusions

I do think that the whole topic of refactoring the declarative options to use hooks, as well as providing a new canEnd (name pending) hook would be worthy of a separate issue/RFC, and seeing as the declarative/dynamic approach should stay in anyways I guess it would still make sense for the PR to move forward.

The normalization function is great, I was hoping for something like that but haven't had the time to look for it yet 😁 That was the main reason I left moveLimit instead of renaming it to maxMoves, because I was missing the convenient entry point for backwards compatibility. Given it exists, making the naming consistent and just providing backwards compatibility (and a deprecation notice) like outlined in your snippet seems the best choice.

PS: on mobile, sorry for potential autocorrect screwups 🙃

[senritsu]

Completely unrelated, docs update for minMoves (and maxMoves if changed) is also still missing from the PR.

[delucis]

OK, so mostly as discussed, here’s what I think this PR still needs.

• Maintain the current behaviour but deprecate moveLimit, by adding something like the following around L150 in flow.ts:

      // Deprecate `moveLimit` without a hard break for now.
      if (phaseConfig.turn.moveLimit !== undefined) {
        logging.info(
          '`turn.moveLimit` is deprecated. Please use `turn.maxMoves` & `turn.minMoves` instead.'
        );
        phaseConfig.turn.minMoves = phaseConfig.turn.moveLimit;
        phaseConfig.turn.maxMoves = phaseConfig.turn.moveLimit;
      }

• We then should make sure we replace any places that check moveLimit to check maxMoves instead.

• Update docs to show maxMoves/minMoves instead of moveLimit.

• Add tests if required to make sure we’re testing the mapping of moveLimit to minMoves/maxMoves.

• Update the ActivePlayers default configs (end of turn-order.ts) to use minMoves/maxMoves.

[senritsu]

After a long-ish break (thanks to surgery) I finally got around to update this PR.

Most open tasks should be addressed, but I am having some trouble writing a test for backwards compatibility of setActivePlayers. The test is commented out atm, I am confused why it does not work and must be missing something fairly obvious. When debugging it, there is no indication of setActivePlayers actually having had any effect.

[delucis]

Hey @senritsu! Hope you’re recovering well — sorry to hear that.

This looks good. Here are a few details and a suggested fix for your commented test.

[delucis]

Looks good to go — just spotted one tiny testing improvement.

[delucis]

@senritsu Thanks for this!