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

Remove unnecessary child FSM creation by only instantiating upon transitions #155

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joepie91
Copy link

@joepie91 joepie91 commented May 6, 2018

This fixes the second issue that I mentioned in #154; it ensures that new child FSMs are only instantiated when transitioning to a new child-containing state, not when eg. handleing a command that's received by a child.

As before, I've created a bugcase to demonstrate it, including an example of the erroneous results: https://git.cryto.net/joepie91/machina-factory-bugcase/src/child-switch

Of note:

  • I'm unsure how to write a good test for this particular behaviour, so I've included no tests with this PR. Existing tests pass, however.
  • I've not included built files in lib/ in this PR due to merge conflict risks.
  • I've left the 'detach and reattach event listeners' bit alone; that still happens regardless of whether a new FSM is instantiated or not. I was not confident enough in my understanding of its purpose in machina.js to do that conditionally as well. Depending on what it's used for, you may want to change that behaviour to avoid potentially missing events.

@joepie91
Copy link
Author

joepie91 commented May 6, 2018

As an aside: this issue originally caused a strange bug in my application where I'd get inconsistent states; a child FSM would transition from state A to state B, but then when handleing a command it would suddenly be in state A again, despite no transition event from B -> A having ever been emitted on the parent FSM (and this wasn't a possible transition in the first place). Presumably there were difference instances of the child FSM, where one of them was in state A and the other was in state B.

This may or may not be indicative of a bug elsewhere. I'm unsure how reproducible the issue is, and I haven't had the time to try it. This PR plus my other one make the bug go away, however.

@ifandelse
Copy link
Owner

Thanks @joepie91. I'm working on catching up on PRs this week (finally have some time off!).

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