Skip to content

Do not depend on Coord's instance methods from serialized board state…#753

Merged
vdfdev merged 1 commit into
masterfrom
checkers-fix
Jan 19, 2021
Merged

Do not depend on Coord's instance methods from serialized board state…#753
vdfdev merged 1 commit into
masterfrom
checkers-fix

Conversation

@mateusazis
Copy link
Copy Markdown
Collaborator

… in checkers.

Fixes #750.

Checklist

@mateusazis mateusazis requested a review from vdfdev January 19, 2021 16:05
@vdfdev vdfdev merged commit 563cc42 into master Jan 19, 2021
@vdfdev vdfdev deleted the checkers-fix branch January 19, 2021 16:34
@vdfdev
Copy link
Copy Markdown
Contributor

vdfdev commented Jan 19, 2021

Thank you!

@leocaseiro
Copy link
Copy Markdown
Collaborator

leocaseiro commented Jan 19, 2021

Thank you @mateusazis and @flamecoals!

@vdfdev
Copy link
Copy Markdown
Contributor

vdfdev commented Jan 20, 2021

@mateusazis Can we get rid of this class entirely? This is a known issue that classes are not serializable, so I am worried another corner case might bring the same issue as we are calling other methods on the game logic. I missed this during review, sorry.

@mateusazis
Copy link
Copy Markdown
Collaborator Author

We can. But, as you said, this doesn't prevent this bug from coming back. The boardgame.io docs warn about it but also don't prevent it. Some ideas that came to my mind were:

  • At compile time: annotate the top-level state interface with a decorator and make sure the class no methods (not sure if this is doable in Typescript).
  • At test time: between turns, serialize and de-serialize the game state when running in Local mode. This would have made our unit tests catch the bug.

Ideally, the 2nd approach would go into the boardgame.io lib.

@vdfdev
Copy link
Copy Markdown
Contributor

vdfdev commented Jan 21, 2021

@mateusazis I am not sure if the first approach is feasible either.

Let's go with the second approach, I think this would also help other developers on the boardgame.io community. We can probably allow for a "strict check" during testing on the boardgame.io Client API. I created #754 to keep track of the work.

@vdfdev
Copy link
Copy Markdown
Contributor

vdfdev commented Jan 21, 2021

For now, I sent you @mateusazis #755 to remove the class from checkers entirely.

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.

Checkers Double Jump Crashes Game

3 participants