Bugfixes for useQuery #243
Conversation
src/hooks/useQuery.ts
Outdated
| }, [executeQuery]); | ||
|
|
||
| return () => { | ||
| isMounted.current = false; |
There was a problem hiding this comment.
Small bug here since the effect runs more times than just for mount & unmount
There was a problem hiding this comment.
Haven't had time to look at this but I'm assuming we'll need a separate effect for managing mounting / unmounting.
There was a problem hiding this comment.
That makes sense to me, keep the effects separate (which also makes them easier to reason about).
There was a problem hiding this comment.
The addition of the request key to the effect was to cause a fetch not only on mount but whenever the query changes. I think this is what users would expect but will leave that for a seperate PR.
src/hooks/useSubscription.ts
Outdated
| return unsubscribe.current; | ||
| }, [executeSubscription]); | ||
| return () => { | ||
| unsubscribe.current(); |
| setState(s => ({ ...s, fetching: true })); | ||
| if (args.pause) { | ||
| unsubscribe.current = noop; | ||
| return; |
There was a problem hiding this comment.
I think the early return here is the reason for the test failures. Basically, waitForNextUpdate expects some asynchronous thing to happen (i.e. a Promise to be resolved when running the useEffect), otherwise it'll never get a signal that the effect completed. You should be able to switch the failing tests to sync tests (remove async keyword and await waitForNextUpdate) and I'd expect them to pass.
There was a problem hiding this comment.
Thanks Parker, I'll investigate tonight 👍
There was a problem hiding this comment.
Looking a little more closely, it appears that the failed tests are related to the effect not re-running when new variables, query, and requestPolicy are received 🤔 Not totally sure why that would be.
There was a problem hiding this comment.
@parkerziegler I've managed to fix the tests by removing the waitForUpdate call. Given tests themselves are checking that a re-fetch is triggered we can get away without waiting for state updates.
As for why those state updates aren't taking place in a test environment, I suspect it's related to the mock implementation of executeQuery.
There was a problem hiding this comment.
Gotcha, will dig into this more tomorrow to confirm. But agree, as long as we're sure that executeQuery is being called again when a new query, variables, or requestPolicy are received, then I'm not too worried about the state updates.
| @@ -130,7 +130,6 @@ describe('useQuery', () => { | |||
| `; | |||
There was a problem hiding this comment.
Think I saw a comment in an email from you @andyrichardson, but yep, to explain the naming behind this file, I basically wanted a way to differentiate it from useQuery.test.tsx. I'd be happy to use a different name. Part of the reason for bringing in react-hooks-testing-library was to make us less dependent on enzyme, to avoid re-assignment of let bindings in tests, and to be able to get the direct values returned by our hooks so we can assert on them (which I think has some value). Perhaps we could specify these two files as useQuery.enzyme.test.tsx and useQuery.rhtl.test.tsx? I'm also fine with removing them if we don't think they have value or we don't like the API, I just find the it a bit easier to work w/ than enzyme.
There was a problem hiding this comment.
Cheers @parkerziegler - I initially asked but then found your PR with the explanation. No need to make changes 👍
I'm pretty sure react-hooks-testing-library is using react-test-renderer under the hood (we're not using enzyme). I agree the reassignment + added complexity is a pain 😑
Changes