Boardgame.io: Custom plugins aren't available in onBegin/onEnd

Created on 6 Feb 2021 · 12Comments · Source: boardgameio/boardgame.io

Hello there! I'm using @delucis' bgio-effects and I'm trying to access it inside a phase's onEnd hook but it comes out as undefined while ctx.random is there alongside the other default plugins.

@delucis created a reproduction after a short chat.

I did a bit more digging writing some failing tests and discovered that there seems to be link with the flush of the plugins. Plugins without flush are available and if a plugin has a flush it's not. I'm not too familiar with the structure of boardgame.io so I mostly dug around with some console logs to track it down.

When this part of the code executes, pluginState contains all the right things, but only the data gets written which removes the API that the plugin provides and thereafter the relevant plugin in state.plugins is just an empty data object.

I'm happy to provide a PR/fix but I'm not too sure what is supposed to happen. Would adding back the rest of the state like here be a simple fix that doesn't disturb anything else?

bug

Source

MathieuLoutre

All 12 comments

Replacing [plugin.name]: { data: newData } on L187 with [plugin.name]: { ...pluginState, data: newData } makes the tests pass (and no other test fails). However, as I mentioned, I've no idea of the repercussions of this down the line or why it was "data only" in the first place.

MathieuLoutre on 6 Feb 2021

Thanks for the detailed report @MathieuLoutre! My understanding is that flush intentionally removes plugin APIs, so that they are not persisted and we don’t try to serialize them and send them to the client [1]. The APIs are created by calling Plugins.Enhance.

For example, the current flow appears to be:

InitializeGame:
- Plugins.Setup creates initial data in state.plugins.
- Plugins.Enhance creates the APIs in state.plugins.
- Plugins.EnhanceCtx applies the APIs to state.ctx, for consumption by game code.
- Plugins.Flush updates state.plugins with any changed plugin data. This removes the APIs for plugins with a flush method.
https://github.com/boardgameio/boardgame.io/blob/ccc9adaf410a748651d1b993f043c6aac6ddca97/src/core/initialize.ts#L44-L64
In the reducer used for moves, events, etc.:
- Plugins.Enhance again creates the APIs from state.plugins to state.ctx (in the event and move reducers)
- The flow reducer calls Plugins.EnhanceCtx when wrapping hooks.
- Plugins.Flush is called again.

I suspect the problem is not flush in this case, but that there’s some missing combo of Enhance/EnhanceCtx although I can’t spot it right now. Perhaps this could help with further debugging? If not, I’ll hopefully have some time to keep digging later this week.

It looks like we should also do this cleanup more consistently, because currently plugins could have an api and no flush method.

delucis on 7 Feb 2021

Thank you, that's a helpful look into the flow. I did notice that the API didn't get discarded if the plugin didn't have a flush method which is why I suggested they might be the issue. This is quite a blocker for what I'm trying to do right now so I will spend some time tomorrow working on this but if you see anything that might be help localise the problem in the meantime, please do report here :)

MathieuLoutre on 7 Feb 2021

So, an additional observation: the log plugin is also undefined at the same point while random and events are both there. There doesn't seem to be a special case for random that I could see anywhere for it to be included when log isn't. random has noClient and playerView but when I add that to a custom plugin it still gets left out.

Adding custom plugins to the list of default plugins doesn't change anything. So it would seem it's not to do with default plugins vs custom. However it doesn't quite explain why a custom plugin with the exact same structure as the random plugin doesn't get included when random itself makes it. (see next comment, the order mattered which I didn't notice at first)

The last time EnhanceCtx gets called before onEnd is triggered, it does go through all the plugins but only random and events have an api variable. So state.plugins is missing some stuff before it gets to that EnhanceCtx. Where EnhanceCtx is called, the relevant data is present in TriggerWrapper and HookWrapper but seems to get lost at some point when it gets to a HookWrapper and never recovers.

MathieuLoutre on 8 Feb 2021

So I think I've gotten a little bit further down the rabbit hole. It seems like it's something related to the plugin order because of the way Flush works.

When the move is called, the context is enhanced and the APIs are present. Later when the flush starts, it goes through the plugins in reverse orders, custom ones first, stripping the APIs.

When it gets to the events plugin, it uses dangerouslyFlushRawState, at this point the random (and immer) plugin haven't been flushed (but all other plugins are stripped).

An update is called as a result of dangerouslyFlushRawState, inside which automaticGameEvent decides it should dispatch an endPhase event. The event is processed and the end of the phase is triggered. random, events and immer are there but all the other plugins have been stripped inside onEnd. The process continues and ends up stripping random etc.

So it seems that triggering the end of the phase through the flushing of the plugins is the issue. The order of the plugins matter to know which one is available. If custom plugins are inserted before the events plugin in the list, the custom plugins are still available during onEnd.

Would there be any adverse effects flushing the events plugin first, then the custom plugins and then the rest?
Something like: [PluginImmer, PluginRandom, PluginLog, ...opts.game.plugins, PluginEvents].reverse() instead of [...DEFAULT_PLUGINS, ...opts.game.plugins].reverse() fixes the issue and all the tests pass (there's a better way to write this but this is for illustration).

MathieuLoutre on 8 Feb 2021

Hi @MathieuLoutre, sorry not to respond sooner — last week was particularly busy. I think you’ve grasped the basic problem. I’m not sure if you’ve also seen #656, which provides the context for reversing the order of the plugins when flushing.

As you noted, when a plugin is flushed, its API is removed, so #656 reversed the array to make the random API available from event hooks (basically it addressed this same bug but for built-in plugins). You’re correct that flushing the events plugin first should fix this particular issue.

The drawback of this fix is that then no plugin can use the events API. (I’m not currently aware of any that do, but someone might have written one.) That said, I’m not sure exactly what the better solution is. I thought about first flushing data and then stripping the plugin APIs, but that doesn’t quite solve it because if you flush plugin A, then plugin B uses plugin A’s API, you need to flush plugin A _again_. In theory we could loop over the plugins repeatedly until all plugins are unused or something, but I don’t know if we want that complexity.

If you have a better idea, I’d love to hear it, otherwise changing the plugin order as you propose seems like a reasonable intermediate fix. Having plugins available in event hooks seems more useful generally than having the events API available in plugins.

delucis on 16 Feb 2021

Hi @delucis, no worries — things have been quite hectic here too. I've ended up temporarily forking the project to apply my patch and be able to use it for demos.

I haven't seen #656 but the reversing made sense (good thing it had a comment!). I don't think there's a very elegant solution: essentially we're looking at a dependency issue here and if there's no way to specify which plugins are used by which other plugin, there's no good way to determine the execution order (except by brute-forcing it like you suggested). Maybe in a future version we can have a plugin field which lists the "required" plugins, and then determine the order based on this?
Alternatively, we could let the user override the default plugin order but that seems fraught with danger and complications.

In the meantime, I haven't had any issues with the plugin order I suggested but it does mean, as you noted, that you can't use the events plugin (which I don't need in the hooks as opposed to bgio-effects).

Sorry I'm not bringing a brilliant solution but it might be a trade-off to make in the meantime while we think up a better API to avoid this altogether?

MathieuLoutre on 20 Feb 2021

Sorry I'm not bringing a brilliant solution but it might be a trade-off to make in the meantime while we think up a better API to avoid this altogether?

Sounds good! The plugin system is designed assuming plugins don’t use each other, which they mostly don’t. The events plugin is kind of an exception because it runs the game code from hooks where using plugins is expected, so I think this change would be a reasonable solution for now.

delucis on 20 Feb 2021

Hi, I'm experiencing the same problem.
Could we just merge the change proposed by @MathieuLoutre in his fork (https://github.com/MathieuLoutre/boardgame.io/commit/d8d7d1ece676a1ab0f79d32fe53cd205f562d516)?

As already mentioned by @delucis: