Use effectScope instead of manually stopping effects

[negezor]

Summary

This PR eliminates the need to manually stop effects, as Vue has an efficient effectScope API. Manual stopping is less readable and consumes more memory unnecessarily, since Vue will still write the effects to scope.

Set of changes

• Removes stops in useQuery.ts and useSubscription.ts
• Replaces stops with effectScope in useClientHandle.ts

This should be a minor change. However, for most users this won't matter because everything basically uses methods directly instead of useClientHandle, which is much more efficient.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6c2c896

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@urql/vue	Minor

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

[JoviDeCroock]

Adding this would break our compatibility with older Vue versions https://github.com/vuejs/core/blob/main/changelogs/CHANGELOG-3.2.md#reactivity - we have to stick to the minimum peerDep version described in our package.json or it will be a breaking change.

[negezor]

Indeed, I didn't notice. Perhaps a major change might be worth considering then? For example, a router for Vue 3 asks for a minimum of [email protected]
https://github.com/vuejs/router/blob/249bf2da5382758fc55a6690c4179a63313f0f24/packages/router/package.json#L111

At the same time, Vue 2 has already reached its EOL https://v2.vuejs.org/eol/

A good option would be to install the minimum version 3.3. We could use native toRef and unref

[yurks]

we don't need to stop watchers explicitly, as watcher automatically stopped when the owner component is unmounted

[kitten]

@yurks I don't want to sound too harsh, but please don't comment assuming that something that's been required is there for no reason 😅❤️
This is relevant for useClientHandle, which, due to its API absolutely has the ability to set up subscriptions that don't happen during mounting and hence aren't cleaned up automatically

urql/packages/vue-urql/src/useClientHandle.ts

Line 134 in 1bb05f5

onBeforeUnmount(() => {

@negezor: I'm a bit concerned about raising the requirement too soon after the recent fixes, since they've been quite important overall.
However, It's clear that Vue's API has had changes that simplify and improve internals tremendously and help us out a lot.
We could consider raising the required Vue version in a separate PR, which schedules a major bump, then make sure most of the changes and overhauls are merged before it's published, and leave a note on the change log that helps people out (basically "downgrade to vXX if Vue 3.1 and 3.2 support is important to you")

[yurks]

@kitten
I have no doubt that there was a strong reason for using this particular code. But I can see also useClient() in deeper usage, which is using getCurrentInstance() and inject() inside, which in turn means, this couldn't be used outside component.

Based on the all above, I can conclude that it's impossible to use this method outside vue context and left a comment here. Am I wrong?

[kitten]

Feel free to browse past issues and discussions regarding this topic. The function instantiates in component setup functions, but is then meant to assist with contexts that are outside of components and Vue's tracking contexts after. This in fact becomes pretty apparent when you try to call useQuery without this in more contexts. You'll see that it straight up errors. You'll also find that without taking care of stop conditions, the subscriptions remain active indefinitely

[yurks]

I may be wrong and I would appreciate it if you could help me realize this, but based on current code, watchers are always connected to component instance. How it possible to stay active, as un-watching should be handled internally by vue? I can see only one possible reason, based on date this code parts was authored - it was developed with beta version of vue 3, where was a number of similar issues.

PS: I just did a little refactoring with care of reactivity handling, and propose to remove flush: 'pre' for watchers there, as it's default setting. But having in mind, nothing is accidental here, I can assume the beta vue3 was used in development stage.

PSS: I'm not the only one who wonders about the necessity of this feature.

[yurks]

@kitten I found your use case - handling several awaits when calling several await useQuery():

 const handle = useClientHandle();
 const resultOne = await handle.useQuery(/* ... */);
 const resultTwo = await handle.useQuery(/* ... */);

But it looks like incorrect code pattern, as it's known vue issue and even vue lint trying to be aware of it.

It's ok to handle such cases manually for sure, but I think it perpetuates the misuse of the framework. Even more, you can't handle possible awaits before calling const handle = useClientHandle();, where the current solution won't work as expected.

I would suggest to document this case and prevent such usage with proposing correct pattern like:

const resultOne = useQuery(/* ... */);
const resultTwo = useQuery(/* ... */);

// sequential execution
await resultOne;
await resultTwo;

// or parallel execution
await Promise.all([resultOne, resultTwo])