boardgame.io icon indicating copy to clipboard operation
boardgame.io copied to clipboard

Move initialization from Master.onSync()

Open radl97 opened this issue 2 years ago • 3 comments

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 from onSync to callers, as Transport 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 (around createMatch), which broke the boardgame.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.

radl97 avatar Oct 02 '22 19:10 radl97

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 :).

vdfdev avatar Oct 07 '22 04:10 vdfdev

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?

radl97 avatar Oct 07 '22 08:10 radl97

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.

vdfdev avatar Oct 10 '22 01:10 vdfdev