-
Notifications
You must be signed in to change notification settings - Fork 88
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
fix: do not fire promise events after state changes #171
Conversation
@matthewp any thoughts on this? |
Is this how Xstate works? |
@matthewp Yes, here is an example https://codesandbox.io/s/suspicious-pare-137g4z |
machine.js
Outdated
.then(data => service.send({ type: 'done', data })) | ||
.catch(error => service.send({ type: 'error', error })); | ||
.then(data => { | ||
if (machine.state.name === service.machine.state.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since every step generates a new machine, wouldn't it be enough to compare the value of machine
versus service.machine
? It would address the case where the state machine has returned to the same state before the promise resolves.
Sorry for the delay @kybarg, can you address @ehuelsmann's review and update the branch? |
b6b599d
to
0952fb0
Compare
@matthewp done |
Could we merge and release this? It seems that the approach was approved, it was just never merged. |
@matthewp, I don't mind updating this PR so you can include it in a next release. I'll need to create a new PR though, because I can't append to this one. Are you fine with that? Does it make sense for me to work on this? |
@ehuelsmann yep, works for me |
3edaa3f
to
0b2caf1
Compare
|
fixes #141