Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove wrapping request args in reactive to fix memory leak #3612

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

negezor
Copy link
Contributor

@negezor negezor commented Jun 18, 2024

Summary

This eliminates the memory leak since reactive always assigned new watchEffects to the static request object.

Resolves: #3507 (comment)

Memory leak appeared in this PR #1151

Set of changes

  • Removes wrapping args in reactive in useQuery.ts and useSubscription.ts

This change is not breaking, since we still do unref as before #1151 PR.

Copy link

changeset-bot bot commented Jun 18, 2024

🦋 Changeset detected

Latest commit: 16915b1

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 JoviDeCroock merged commit 5d9d134 into urql-graphql:main Jun 18, 2024
13 checks passed
@github-actions github-actions bot mentioned this pull request Jun 18, 2024
@yurks
Copy link
Contributor

yurks commented Jun 18, 2024

this is completely breaks reactivity support for variables, like { variables: { somevar: ref('value') }}
how could it get into production?

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Jun 18, 2024

@yurks mate chill... first and foremosta tone like that when not adding tests/... is really not appreciated

I'll go through the types/... in depth tonight and get to know Vue to get everything fixed hollistically

@yurks
Copy link
Contributor

yurks commented Jun 18, 2024

(how you can identify the tone by text question? PR was approved and immediately released)

that's not about types but about reactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible memory leak when using multiple queries on the same page
3 participants