search

urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.54k stars 444 forks source link

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

Closed negezor closed 2 weeks ago

negezor commented 2 weeks ago

Summary

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

Resolves: https://github.com/urql-graphql/urql/issues/3507#issuecomment-2174017806

Memory leak appeared in this PR https://github.com/urql-graphql/urql/pull/1151

Set of changes

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

changeset-bot[bot] commented 2 weeks ago

🦋 Changeset detected

Latest commit: 16915b1f52228d86787b8ecb0f18941c4d7d13e0

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

yurks commented 2 weeks ago

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

JoviDeCroock commented 2 weeks ago

@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 commented 2 weeks ago

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

that's not about types but about reactivity.