(vue-urql) - Add Tests #1153
(vue-urql) - Add Tests #1153
Conversation
from useSubscription.test
|
|
|
||
| const query = await useQuery({ | ||
| query: `{ test }`, | ||
| // pause: true, |
There was a problem hiding this comment.
Oh that's interesting! I think this is because we can liken this behaviour to suspense in React, so once useQuery's promise resolves and we subscribe again we don't have a result, since no cacheExchange is used.
Adding the appropriate exchanges however is not what we want though. While it would fix this, the cache doesn't cache errored results for instance, so instead we'll have to copy some code from react-urql's useQuery where the result is cached just for the suspended stream for the next rerender after suspending.
I can take a look at fixing this
There was a problem hiding this comment.
My main problem here was that I wasn't sure what the use case of making it " thenable" is, so I wasn't sure what an appropriate fix would be.
Am I understanding correctly that this is supposed to support Vue's version of Supsense?
There was a problem hiding this comment.
Yea, I think I also don't quite know enough unfortunately quite yet to fix this. So it is indeed to support Vue's version of suspense, so we'd like to await the first result that executeQuery may yield but during normal execution we'd also like future results to still update the component.
I'm not quite clear on how the component lifecycle functions once Vue suspends, so it'd be interesting to know whether the lifecycle aborts when it does.
I think we may get into some trouble here with our isThenable flag, but essentially the idea was always to return the promise, resolve it on a result, and otherwise still keep a subscription active to update the result refs passively.
There was a problem hiding this comment.
Gotcha. I'll think a bit about it as well.
|
|
||
| expect(query.fetching).toBe(true); | ||
|
|
||
| subject.next({ data: { test: true } }); |
There was a problem hiding this comment.
I'm not too sure if this test is adequate for polling. Maybe we can use some timer mocks in the executeQuery's mockImplementation so it really sends a couple of data points after timeouts?
then again, would that really improve the tests? Not sure.
There was a problem hiding this comment.
I think this is adequate, since all it'd have to support is results being actively pushed into the useQuery subscription
|
@kitten Suggestion: we merge these tests as they are, with the skipped one, just so we have some basic checks in place for the rest of the functionality. Then we can work on solving the issue with that Promise for Suspense without breaking stuff accidentally. |
There was a problem hiding this comment.
This is some awesome work 🎉 I agree @LinusBorg let's merge this as is, having tests in place is already awesome
Summary
This PR adds tests for:
useMutationuseQueryuseSubscriptionSet of changes
jestto the package's tsconfig type option in order to stop TS from complaining, this should not have any impact outside of the vue-urql package.useQuery().then()behavior, and I'm not certain what the desired behavior here should be. Please see the code commentsThis only adds tests, so I didn't create a changeset. I can provide one if that's still required for this kind of PR.