Open neatnerd opened 6 years ago
Hey there @neatnerd, really appreciate the contribution!
I'm happy to accept async support, but there's a couple things I'd like to see in this PR before I merge it:
Just as non-async action creators have a version which gives the handler just the payload an a version which gives the handler the entire action (e.g. .case
vs. caseWithAction
), the async version should as well. Up to you what you want to name the two: perhaps .caseAsync()
and .caseAsyncWithAction()
.
It's not clear to me why .casesWithAsyncFailure
needs to have its own function. Presumably the use here is that you handle all the successes individually, but capture all the failures together, e.g.
.case(loadUsers.done, users => { /* ... */ })
.case(loadGroups.done, groups => { /* ... */ })
.casesWithAsyncFailure([loadUsers, loadGroups], () => { /* ... */ });
but this breaks visual symmetry a bit, in that .done
is written out in the success cases but .failed
is not written out in the failure cases. I think in this case it would be clearer to just use the existing .cases
, e.g.
.cases([loadUsers.failed, loadGroups.failed], () => { /* ... */ });
as this maintains parity between writing .done
/.failed
and also doesn't increase the size of the API.
Perhaps there's another reason to use this that I'm not seeing, but it seems to me that this method is superfluous.
Supposing there is a good reason to have .casesWithAsyncFailure
, it should also have separate payload and action versions as mentioned above. Furthermore, it should be given types so that it works even if not all the error types are the same, as is done with the type definition for .cases()
and .casesWithAction()
.
Tests should be written for any new methods.
Thank you very much for a prompt feedback. Before I proceed - couple of comments/questions:
Hi again,
You're right that a test probably isn't strictly necessary to check correctness since as you said it is a simple wrapper. But a benefit of writing a test is to make sure this API plays nicely with action creators defined using actionCreator.async()
from typescript-fsa. Since the point of async handlers here is to integrate with typescript-fsa, it's worthwhile to write out a test to demonstrate that this actually happens and also to give an example of doing so.
I suggest to remove package-lock.json
and rely on yarn.lock
instead, since it is already present.
Added support for ease of use