-
Notifications
You must be signed in to change notification settings - Fork 45
add some basic testing for useAsync #15
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
Conversation
Hey, really thanks a lot for working on this :) I was a bit lazy and didn't implement tests yet. Having a few initial tests will probably motivate me to write more ;) BTW, I could merge this PR as is (because it's better than no tests) but I'd rather avoid making the tests depending on fetch, as this library can work with any async code. Also, not sure the "initialProps" here is very useful (not yet used to this lib but it's what I understand). I think you could write something inline like this: const { result } = renderHook(() => useAsync(async () => {
return fakeResult;
})); Do you want to me merge as is and see myself how to simplify it, or give it a try yourself before merging? |
I think you are right on both points. I've removed jest-fetch-mock and the Let me know if there is anything else. |
I mentioned I think hooks are limited if we cannot somehow trigger them from callbacks like event handlers. Before this, we were storing things like What are your thoughts? |
Hey Paul, Sounds like you have thought about this a lot already. We're actually in the process of consolidating these libraries in Async Library. Sébastien and Daishi are both involved in this. It would be great to have your feedback on the design. As far as I'm concerned anything is on the table, if it helps cover all use cases in an elegant way. Also check out React Async if you haven't already. |
@ghengeveld ooooh, thanks for the heads up, I look forward to having a look at this. I keep on thinking it should be a state machine with proper state transitions. The only problem is all these packages sound really similar :). I always get Now I've got |
Thanks @dagda1 will review the PR as soon as I have time (not much this week :/) Will also check your blog post to see what's it about and the comments.
You don't need to call execute if you don't need to trigger the fetch / async execution lazily. This is perfectly fine to me. Look at how apollo hooks handle mutations, it's exactly the same in my lib. In non-lazy fetching cases you don't need to call execute, but it's still there to trigger manual re-executions (think refetch method in Apollo, but couldn't name it refetch because async is not necessarily a fetch) I don't put loading booleans in Redux for a while :) |
yes, that is what i am saying, the only reason to call execute outside of
an effect like on an event handler for example. this package was the first
one i saw that offered a way of doing this. all the other sax where for
effects only
On Mon, 21 Oct 2019 at 12:43, Sébastien Lorber ***@***.***> wrote:
Thanks @dagda1 <https://github.com/dagda1> will review the PR as soon as
I have time (not much this week :/)
Will also check your blog post to see what's it about and the comments.
I got a lot of feedback on this post and a lot of people are saying that
you would not need the execute function that is returned from useAsync.
Most are saying you can do this with state changes but I disagree. Others
say that calling a callback in a hook is not what hooks are about as they
are lifecycle only.
You don't need to call execute if you don't need to trigger the fetch /
async execution lazily. This is perfectly fine to me. Look at how apollo
hooks handle mutations, it's exactly the same in my lib. In non-lazy
fetching cases you don't need to call execute, but it's still there to
trigger manual re-executions (think refetch method in Apollo, but couldn't
name it refetch because async is not necessarily a fetch)
I don't put loading booleans in Redux for a while :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15?email_source=notifications&email_token=AAA44OBHOIM5FG6BUIESJG3QPWIXJA5CNFSM4JCQIKYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB2A56Y#issuecomment-544476923>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA44OFNEO7KPJR72FIDDOLQPWIXJANCNFSM4JCQIKYA>
.
--
Cheers
Paul Cowan
Cutting-Edge Solutions (Scotland)
blog: http://thesoftwaresimpleton.com/
website: https://cutting.scot/
|
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.
That looks great., thank you
If you have time I think it would be better to not add to much types, the type inference should be able to work fine.
Otherwise I'll merge this and see myself if this can be simplified a bit
Please feel free to simplify or whatever. Thank you for the feedback |
Thanks @dagda1 looks great :) |
I was showing this hook to some coworkers and they were offput by the lack of tests.
This PR sets out to address this by adding some basic testing around
useAsync
.If there is anything you would like me to change, then I value your feedback.
Thanks for the
react-async-hook
, I've learned a lot looking at this hook.