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

add lobby machine #25

Merged
merged 9 commits into from
Jan 4, 2024
Merged

add lobby machine #25

merged 9 commits into from
Jan 4, 2024

Conversation

richpjames
Copy link
Contributor

@richpjames richpjames commented Nov 14, 2023

This is an attempt at introducing an XState state machine to control the lobby behaviour as a proof of concept to see what using XState might look like if used across the whole app.
I have opted to use XState 4.x.x as it is better documented and I have more experience with it

image

@richpjames richpjames force-pushed the lobby-machine branch 3 times, most recently from 86303f2 to df27551 Compare November 20, 2023 10:14
@richpjames richpjames marked this pull request as draft November 20, 2023 10:14
@richpjames richpjames marked this pull request as ready for review November 20, 2023 13:07
Copy link
Contributor

@Gweaton Gweaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never used xstate, but this feels like a good use of it, and is definitely a useful thing to understand.

I've left a couple of comments, they might be a result of my lack of recent context but hopefully they're helpful.

Copy link
Member

@yndajas yndajas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks great, but I've added a few things it might be good to clean up or think about before we merge this

@lookupdaily lookupdaily force-pushed the lobby-machine branch 4 times, most recently from 6f86fa1 to 9f05665 Compare December 21, 2023 13:55
richpjames and others added 9 commits January 4, 2024 14:42
It is useful to seperate logs that we want to keep in the application with 'debugging' logs we put in temporarily.
Previously we only had Playwright tests but we will soon have unit tests too
so we need to update the script/test file and package.json
We also add lobby.typgen.ts which is imported via the
'tsTypes' key in the 'createMachine' call on line 43.
This is autogenerated by the VSCode XState extension and, despite guidance in the XState docs,
should be committed in order for CI to pass.
I think we should use this class as the intermediary between the
state machine and the rest of the application.
- If a player leaves ensure the correct action takes place
- Change the state back to OnePlayer if the player count ever reaches 1

Add tests for changing state on player leaves

If we remove the cap on two players, we should add an additional test for the condition (which checks that there is only one player left before changing back to `OnePlayer` state.
We should also add tests for the number of players on action 'playerLeaves'.
We were previously switching to GameStart when two players had joined the game. Instead we are displaying a button on two players joining to make 'playerClicksStart' a user triggered event to transition to 'GameStart'

Add a new test that more than 2 players can join, and that 'GameStart' state only exits if one player is left.
Test that playerLeaves action removes players from the context in each state
We only want to switch to OnePlayer state if there is one player left - same as in GameStart state
This was referenced Jan 4, 2024
Copy link
Member

@yndajas yndajas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock. Can refactor later if needed

@richpjames richpjames merged commit 1c0c953 into main Jan 4, 2024
@richpjames richpjames deleted the lobby-machine branch January 4, 2024 15:23
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.

4 participants