Rework leave-game around an explicit player roster in ctx - #1326
Conversation
Removal was implemented by scrubbing nine fields of ctx and keeping a _removedPlayers tombstone, because playOrder is regenerated from ctx.numPlayers at every phase start. Seven further sites had to re-subtract that list whenever a player set was rebuilt. A following commit makes playOrder carry its own membership forward, which removes the need for the tombstone entirely. Strip the old mechanism first so the replacement lands against a clean slate. leaveGame is unreleased, so no published behaviour changes. The lobby endpoints, the onPlayerLeave hook and the PLAYER_LEAVE action are untouched; only their effect on turn order is removed. events.removePlayer(playerID) and its documentation section are dropped for good, not restored by a following commit. Three robustness fixes #1258 made inside InitTurnOrderState are deliberately kept: the play-order stringify, the playOrderPos out-of-range clamp, and the empty-play-order currentPlayer fallback. All three are reachable without the removal feature.
playOrder served two purposes — the set of players in the match and the order they take turns in this phase — and TurnOrder.CUSTOM writes a phase-scoped subset into it, so it cannot carry membership. ctx.players now holds the membership and playOrder defaults to it, keeping its existing meaning. No behaviour change: ctx.players is fixed at creation in this commit, so every game sees exactly what it saw before. A following commit makes player removal update ctx.players.
PLAYER_LEAVE now removes the player from ctx.players. Because
InitTurnOrderState re-seeds playOrder from ctx.players at the start of every
phase, that is what makes a removal persist -- which is why no _removedPlayers
tombstone is needed.
ctx.playOrder is now always a subset of ctx.players. A turn.order.playOrder
that names a departed player would otherwise put them back in the rotation at
the next phase boundary, and TurnOrder.CUSTOM takes a static literal, so the
game has no way to drop them itself. The same applies to the value and
currentPlayer forms of turn.activePlayers, which are re-applied at every
StartTurn. This narrows one undocumented tolerance: a custom play order naming
an ID that was never in the roster is now dropped rather than passed through.
ctx.numPlayers deliberately keeps its creation-time value. Read
ctx.players.length for the live roster.
Stale ctx._prevActivePlayers and ctx._nextActivePlayers entries are made inert
where they are consumed rather than scrubbed on removal, which is where most of
the previous implementation's bulk went.
Two paths stay outside the contract, because without a tombstone the framework
cannot tell "has left" from "was never in the roster", and a game may name an
ID outside the roster today: endTurn({ next }) and the array form of
setActivePlayers both set the players they are given. A game that names an ID
explicitly owns that ID. Documented in Lobby.md.
GetPlayers falls back to deriving the roster from ctx.numPlayers, so a match
persisted before ctx.players existed still loads.
|
Yeah, I read though all this. This is right, we should have an overall list, but we should keep track of the original list of players to compare against with cause the information is lost as who left. The overall Shape's right, ship the revert. One blocker: The filter's reasoning is right, it just can't tell a departure from an ID that was never here. Suggestion: keep One decision: |
Draft, and deliberately so — this changes public API, so I'd like to agree the shape
before polishing it. It also reworks part of #1258, which only just landed, so I'd
very much like @Rupesh-ark's read on it.
The filtering mechanism doesn't sit well with the existing design
ctxhas never held a list of players. It holdsnumPlayers, andplayOrderisrebuilt from
numPlayersat the start of every phase:So a player taken out of
playOrderis back at the next phase change. #1258 handlesthat with a
ctx._removedPlayerstombstone that every consumer subtracts again:A tombstone is a negative list, so it is only correct while every reader remembers to
subtract it. Nothing in the type system says a new reader of
playOrdermust.It also blocks things we might want next
Each of these needs to change who is playing:
With a tombstone there is nothing to change — only a list of who has been struck out.
The alternative
The first commit reverts the turn-order half of #1258. The rest replaces it with one
new field,
ctx.players— the players in this match, in seat order:metadata.playersctx.playersctx.playOrderplayOrderis seeded from the roster instead of from a count, and constrained to it:Removing a player is then a removal rather than a filter, and
RemovePlayerdrops from65 lines to 30 — the deep scrub of
_prevActivePlayers/_nextActivePlayersgoes away,because both are made inert where they are read instead.
GetPlayers()falls back to deriving the roster fromnumPlayers, so matches persistedby 0.50.x keep working.
Commits
revert(core)drop the turn-order half of the leave-game featurerefactor(core)givectxan explicit player listfeat(core)remove a leaving player from the matchThe middle commit is pure structure: I diffed its behaviour against
mainacross sixscenarios and the resulting states are identical.
Two things I left alone on purpose
endTurn({ next })and the array form ofsetActivePlayersstill set whatever playerID you name, including one who has left. Without a tombstone the framework can't tell
"has left" from "was never in the roster", and boardgame.io tolerates the latter today.
Documented rather than changed.
flow.tsEndTurnreturns the wrongstatein thearg.removebranch, so removingthe last player silently does nothing. Pre-existing since Add pass event #492 (2018), out of scope here.
Where this goes next
This PR stands on its own — it adds no features and
leaveGameends up behaving exactlyas it does today. What the roster makes possible afterwards is written up separately, so
none of it has to be bought into here: #1327 (seating), #1328 (lobby lifecycle),
#1329 (tidy-up).
pnpm testandpnpm run lintare green — 43 suites, 911 tests, 100% statements andbranches.