(vue) - Fix Vue Suspense by making thenable reactive #1159
Conversation
🦋 Changeset detectedLatest commit: 4cf83ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| () => { | ||
| const newRequest = createRequest<T, V>(args.query, args.variables as any); | ||
| if (request.value.key !== newRequest.key) { | ||
| request.value = newRequest; | ||
| } | ||
| }, | ||
| { | ||
| immediate: true, | ||
| flush: 'sync', |
There was a problem hiding this comment.
sync flushing is problematic and usually not needed. In this case, it could lead to unnecessary request re-runs:
const query = ref(`...`)
const variables = ref({ foo: 'bar' })
const result = useQuery({ query, variables })
query.value = `new query`
variables.value = { foo: 'baz' }The above will create a new query 3 times, where 2 would be enough:
- the initial request from calling
useQuery - user changes query ref -> new request created synchronously
- user changes variable ref on the very next line - > new request created again, immediately replacing the request that was just created.
the default flush behavior (pre) should work fine here.
| const unsubscribe: Ref<() => void> = ref(noop); | ||
|
|
||
| watch( | ||
| [args.query, args.variables], |
There was a problem hiding this comment.
This is problematic, as it only watches for shallow changes of the ref.
A user could do this:
const variables = ref({
foo: 'bar'
})
const query = useQuery({ query, variables })
variables.value.foo = 'baz'and would expect for the effect to re-run, but it won't. As an immediate remedy, we can use the deep: true option. But that's not perfect either - in cases where they query isn't a string but an actual gql object, the whole object would be traversed needlessly in order to deep watch for changes.
I think we can refactor from these individual property watches, but I'll write that up later.
| }, | ||
| { | ||
| immediate: true, | ||
| flush: 'sync', |
There was a problem hiding this comment.
same as above: sync behavior is problematic and should be removed.
| [source], | ||
| (_, __, onInvalidate) => { | ||
| if (!source.value) { | ||
| return unsubscribe.value(); |
There was a problem hiding this comment.
Maybe I'm missing something, but the onInvalidate callback should already take care of unsubscribing in either case. we can just return here without unsubscribing again.
There was a problem hiding this comment.
this is here because of pausing. executeQuery can update source.value and the watcher may set it to undefined when everything's paused, so that executeQuery can explicitly take precedence over the watcher's subscription being paused.
| let result$: Promise<UseQueryState<T, V>>; | ||
| if (fetching.value && source.value) { | ||
| result$ = pipe( | ||
| source.value, |
There was a problem hiding this comment.
Disclaimer: This might be my lack of understanding for wonka speaking:
What happens if, while the request of the first source is still running, a side-effect i.e. changes variables and a new source & request is created?
- Would this promise never resolve?
- Would this promise still resolve to the original requests value?
- Would this promise somehow resolve to the second request's value, as we concat sources on line 134?
The third behavior is what I think should happen but I can't tell if it does, and the tests don't cover it yet.
There was a problem hiding this comment.
It's the second case right now, but potentially we can now try shoehorning the third case in.
|
Awesome! Generally I made the watchers more verbose as I found it's easier to make it work this way first and I'm sure we can trim it back down later ✌️ Good shout on the promise behaviour. I'm not sure if we do want to support a case where the source is a moving target as that could maybe lead to some gnarly looping behaviour when user code goes wrong, but generally I agree. We can try to do that, yea Edit; merging this but I expect we'll improve on this in separate PRs |
Summary
This should fix the thenable in
useQuery's Vue Suspense implementation by making it more reactive.cc @LinusBorg