Added error handling in hook callback #21
Conversation
…hrow if value is not valid
Codecov Report
@@ Coverage Diff @@
## master #21 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 18 28 +10
Branches 0 1 +1
=====================================
+ Hits 18 28 +10
Continue to review full report at Codecov.
|
| @@ -138,6 +138,7 @@ Renders a test component that will call the provided `callback`, including any h | |||
|
|
|||
| - `result` (`object`) | |||
| - `current` (`any`) - the return value of the `callback` function | |||
| - `error` (`Error`) - the error that was thrown if the `callback` function threw an error during rendering | |||
There was a problem hiding this comment.
Technically you can throw anything (not just an Error instance), so I'm not sure if this should be any as well?
There was a problem hiding this comment.
I am not knowledgeable enough with typescript, but I think even if someone would throw, it would be an instance of Error as they would extend Error to create a custom error in case they wanted to. So, I think this should be fine unless someone with knowledge of typescript differs in opinion.
| @@ -7,7 +7,8 @@ export function renderHook<P, R>( | |||
| } & RenderOptions | |||
| ): { | |||
| readonly result: { | |||
| current: R | |||
| readonly current: R, | |||
| readonly error: Error | |||
There was a problem hiding this comment.
Again, technically you can throw anything, so maybe this should be a bit looser?
Also, typescript isn't really my thing, so not sure if readonly usage is correct here and the fact that either current or error will be undefined depending on whether the hook threw or not (I think types are nullable/undefinedable by default in Typescript?)?
| expect(result.current).not.toBe(undefined) | ||
| expect(result.error).toBe(undefined) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Are there any other test cases or usage patterns we'd want to to see being tested?
|
@dhruv-m-patel I'd be interested in your review and feedback on this one. @otofu-square likewise for the type definition changes. |
There was a problem hiding this comment.
I pulled the changes using git+ssh://git@github.com:mpeyper/react-hooks-testing-library#error-updates and verified this to work as outlined in your tests.
This is the first time I ever posted an issue on a package I am using and the maintainer/owner showed the willingness to go an extra mile and improve the package. This is very much appreciated. Many thanks!
|
@dhruv-m-patel please submit a PR adding yourself as a contributor with a
I hope you continue to involve yourself in OSS despite the previous bad experiences. As maintainers, sometimes requests go against the core principal's of the package or are just too difficult/time consuming to justify a niche use case, but where possible we should always be willing to consider new ideas for the community. |
|
@jpeyper I feel like you should get some sort of contribution recognition for this as wel, given the idea to use a getter was 100% yours. |
Thanks :) |
Thanks for encouraging.
definitely 👍
Done! |
|
I'd really like to get someone with more experience with Typescript to tell me I'm not doing something dumb with the types before I merge this. |
|
Well, I don't think I'm going to get that Typescript review I'm after, so I guess we'll jsut have to fix it quickly if I screwed them up... |
What:
Capture errors that are thrown by hooks so they can be asserted in tests and are more visible in test failures.
Why:
Resolves #20
How:
result.errorthat returns any thrown value from the hook callbackresult.currentto be getter that can throwresult.errorif value is not validresult.currentis noreadonlyand cannot be overridden by usersChecklist:
I'll comment the parts I'm not sure about for review.