feat(events): Don’t leak stage events across turns & allow self-ending turns/phases

[cristiands7]

Closes #899
Closes #526
Closes #558

An important side effect of this change is that it opens up the possibility to produce infinite loops using effects on hooks. We should add some check to prevent this, or at least warn about it.

We should also evaluate the possibility of remove ctx from endIf, because the developers expect its value to be up-to-date and it can be confusing.

---

Edit (by @delucis): This does not yet address #769 because that needs endIf to run after activePlayers has been updated.

[delucis]

Thanks for this @cristiands7 — great to see those issues all targeted by a single PR :-)

One thought: do you think we could use the value of ctx when Events is instantiated to track how many turns have ended? That way we could also protect against infinite loops by setting a maximum number of turn endings and bail when we hit that. For now it could be some arbitrarily large number like 10 * ctx.numPlayers, but we could potentially also make it configurable just in case someone wants to increase the threshold.

(Side note: in this kind of scenario it could be helpful if there was an API for plugins declaring the action invalid along the lines of the INVALID_MOVE constant moves can use — it might be better to have a harder break like that rather than just ending 20 turns and logging a warning.)

[cristiands7]

@delucis I don't like the name storeMetadata. Can you think of a better name?

[delucis]

@delucis I don't like the name storeMetadata. Can you think of a better name?

Maybe something like updateTurnContext?

[delucis]

Thanks @cristiands7! Here are a few more comments/details.

[cristiands7]

@delucis I would like to wait for #963. So I can use the new isInvalid option in this PR. Do you think this is a good idea?

[delucis]

@delucis I would like to wait for #963. So I can use the new isInvalid option in this PR. Do you think this is a good idea?

Sure! That makes sense.

[delucis]

@cristiands7 Just merged #963 so you should be able to merge that in to this branch and use the new isInvalid API. We’ll probably want to set some internal flag in the events state when the behaviour is invalid, and then check that with the new isInvalid method. Let me know if you have any questions.

[cristiands7]

@delucis I'm happy that you can submit the pending changes. I've been a bit busy these days and couldn't take the time to do it. Thanks.

[delucis]

@cristiands7 Thanks — I didn’t want to jump on your toes, but I had a bit of time and thought it would be best to help with this PR, which should be a nice fix for some users.

[delucis]

@vdfdev Do you want to take a look at this before I merge it?

It basically adds support for patterns where phases/turns self-end via their onBegin method as long as that doesn’t trigger infinite loops (#526, #558) and prevents stage events leaking into the next turn (#899).

Changes addressed

[vdfdev]

Looks great! For a moment I thought you had solved the halting problem https://en.m.wikipedia.org/wiki/Halting_problem hahaha, but having a theshold on number of actions per turn LGTM, just left a comment

[cristiands7]

@delucis About #769: the new changes allow to end a turn when all players exit a stage using endIf hook. But it is not allowed using activePlayers (or another value of ctx). Maybe it would be better to close this issue and create another one with a better description.

[delucis]

@cristiands7 I think #769 is clear enough about wanting to check activePlayers is empty from endIf, which we’re not addressing in this PR, so I’m happy leaving it open for now. We can always add clarifying comments on that issue.

[delucis]

Thanks for the work getting this started @cristiands7! This is now released on npm as part of v0.46.0 🎉