Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move initialization from Master.onSync() #1099

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

radl97
Copy link

@radl97 radl97 commented Oct 2, 2022

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.

@vdfdev vdfdev marked this pull request as draft October 7, 2022 04:31
@vdfdev
Copy link
Contributor

vdfdev commented Oct 7, 2022

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

@radl97
Copy link
Author

radl97 commented Oct 7, 2022

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?

@vdfdev
Copy link
Contributor

vdfdev commented Oct 10, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants