boardgame.io
boardgame.io copied to clipboard
Move initialization from Master.onSync()
NOTE: Tests fail. This could be an issue, but the "attached" code might help understand an issue.
I have tried to complete a TODO, basically extract this code:
class Master {
async onSync(..., numPlayers = 2) {
...
// If the game doesn't exist, then create one on demand.
// TODO: Move this out of the sync call.
if (state === undefined) {
... Initialize and save state, which uses numPlayers
}
...
}
}
However:
- Many tests rely on the behaviour (with non-trivial way to include it in at least 1 test), but it seems that not many real code flows bump to this case (with the exception of LocalMaster. I have added a fix for that, see my commit).
- The numPlayers is only known in the
onSync
call, and I have failed to extract it fromonSync
to callers, asTransport
should not know much about the game logic.
Why
- This was a TODO
- My preface is that I have modified boardgame.io to add bots. It is not yet stable, but I'm open to discussions. Thus, I have expanded the
boardgame.io/server
code (aroundcreateMatch
), which broke theboardgame.io/master
code (due to this code sample), which broke client-only functionalities (Local bot games). This propagation is silly, hence I wanted to move out server dependencies.
I think that this dependency cycle is unhealthy. I tried fixing the issue (removing server code from the client), but this is only a work-around. Any question, help or feedback is appreciated.
Hi, I converted this PR to Draft until you finish having the tests passing. This is in order for us to keep it organized, I want to make sure all PRs that are ready to be reviewed to be done so quickly :).
Hi! As I've mentioned, tests failing is not an issue (I could delete them, so it is visible :D ). Tests depend on an (I think) internal feature.
I wanted to ask how should I go about fianalizing the PR. Which tests functionality with external visibility? There are many-many tests broken like this, but some of them only because the tests in the setup phase do not get initialized.
Can you help me with this issue?
Sorry, we dont delete tests, it is expected that you figure out the tests for any change you make. If you want to ask more specific questions about a specific functionality, I am happy to help, but I don't have the time to debug everything for you.