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

if side effects of action throw error, state of model will be wrong #7

Closed
WangLarry opened this issue Oct 27, 2021 · 5 comments · Fixed by #10
Closed

if side effects of action throw error, state of model will be wrong #7

WangLarry opened this issue Oct 27, 2021 · 5 comments · Fixed by #10
Assignees
Labels
bug Something isn't working

Comments

@WangLarry
Copy link

Describe the bug
In processQueue function, catch error of actions, then run following codes. It means that side effects are not ran totally, state is not complete.

To Reproduce
In custom validator function, throw error or timeout.

@galhavivi
Copy link
Owner

galhavivi commented Oct 27, 2021

Thank you for sharing love ☺️

True. I guess it’s a normal javascript behavior. You get notified by error that the action has failed (usually means that you need to fix your handlers or side effects which you supplied to cofi), and its only normal that it won’t continue to the side effects like “afterChangeValue” if changeValue for example has failed.

I think that if we do run the effects if the action fails, it can cause unexpected behavior in the system that uses cofi, since it expects the afterChangeValue to run after the value has changed successfully.

Do you have an alternative way to handle it?

Perhaps we can add a note in the error which says that side effects didn’t complete?
WDYT?

@WangLarry
Copy link
Author

WangLarry commented Oct 28, 2021

MDN web docs say:
The Promise.all() method takes an iterable of promises as an input, and returns a single Promise that resolves to an array of the results of the input promises. This returned promise will resolve when all of the input's promises have resolved, or if the input iterable contains no promises. It rejects immediately upon any of the input promises rejecting or non-promises throwing an error, and will reject with this first rejection message / error.

In comparison, the Promise returned by Promise.all() may be more appropriate if the tasks are dependent on each other / if you'd like to immediately reject upon any of them rejecting.

It means that one of promise is failed, others promise will stop immediately. So the result is not just ignore this promise, the state is unexpected totally.

@WangLarry
Copy link
Author

The common scene: async validator throw error or timeout.

Maybe we can just deal this scene at first.

@WangLarry
Copy link
Author

One method:
Before run action, store the snapshot of current state. Then run actions and optimistic update state. If throw. error in actions, restore the snapshot in catch block, report errors to user, continue run following actions.

@galhavivi
Copy link
Owner

@WangLarry
Great solution, Thank you :)
Please check out the open PR that is link to this issue, and let me know what do you think.

@galhavivi galhavivi added the bug Something isn't working label Oct 28, 2021
@galhavivi galhavivi self-assigned this Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants