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

useQuery not returning stable references #2701

Closed
3 tasks done
sarink opened this issue Sep 23, 2022 · 8 comments · Fixed by #2736, #2740 or #2741
Closed
3 tasks done

useQuery not returning stable references #2701

sarink opened this issue Sep 23, 2022 · 8 comments · Fixed by #2736, #2740 or #2741

Comments

@sarink
Copy link

sarink commented Sep 23, 2022

Describe the bug

I hope that I'm simply misunderstanding or have misconfigured something, but I can't seem to figure out why objects inside useQuery().data.MyQuery.someList[index] are changing references. This means they cannot be memoized, and the application rerenders unnecessarily

  • On initial load, the TodosList and every Todo renders three times
  • Upon creating or updating a todo, each individual Todo in the list rerenders

Thanks :)

For skimming (if you don't want to pull the repro repo):

Screen Shot 2022-09-23 at 4 05 03 PM

urql-memo

Reproduction

sarink/urql-perf#2

Urql version

^2.2.3

Validations

  • I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
  • Read the docs.
  • Follow our Code of Conduct
@JoviDeCroock
Copy link
Collaborator

We have gone over this quite a few times in issues/discussions, we do our best to provide stable references but at the end of the day it isn't the most important thing. i.e. a re-render isn't the expensive thing, memoizing everything and doing deep equality might be a lot more expensive if we'd do that on every query.

The best bet for us is to ensure you have the most up-to-date data and you can optimize the re-render process yourself by means of memo and useMemo, also a re-render isn't expensive as long as you aren't doing the most expensive operation, which is touching the DOM.

Other issues #425 (comment)

@sarink
Copy link
Author

sarink commented Sep 30, 2022

@JoviDeCroock the code is fully memoized. But if the object references keep changing, memoizing has no effect.

It sounds like you're saying this is by design, not that big of a deal, and that if you're using urql, you should just expect rerenders?

@sarink
Copy link
Author

sarink commented Sep 30, 2022

I don't really accept that this impossible or that it's it is unimportant. I just rebuilt the same demo with apollo client and it behaves exactly as you'd expect, the memoization works and rerenders are prevented:

Apollo client repro: sarink/urql-perf#3

urql-apollo-memo

@kitten
Copy link
Member

kitten commented Sep 30, 2022

Just to send an update, this could be an issue with Graphcache losing the reference to the last result, but I haven't looked into this just yet

@sarink
Copy link
Author

sarink commented Oct 3, 2022

This is destroying our rendering performance, and with memoizations having no effect, I don't think there's anything consumers can do about it? I'm surprised more people haven't noticed/this isn't of high importance

Is this something that could be fixed in v3? Or a regression that could be fixed by downgrading?

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Oct 3, 2022

I mean, you can do a short-term fix if that is what you are looking for by leveraging JSON.stringify() as your memoization artifact, alternatively you can try to downgrade but afaict we haven't changed anything to this logic.

useMemo(() => result.data.todos, [JSON.stringify(result.data.todos)]

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Oct 4, 2022

Some initial observations from going through this

  • we see three re-renders and the last adds __typename to the selection-set
  • when we add __typename to the query we only see two re-renders

so I guess we can boil this down to two distinct issues, one where we aren't removing __typename correctly and secondly one where we aren't marking the array as untouched and re-using it for a subsequent result.

@JoviDeCroock
Copy link
Collaborator

@sarink we were just testing this and noticed you are using an old version of graphcache, when updating to the latest version we do see this working....

Console output of only 1 re-render happening in the OP reproduction

However we did notice that there's a bug with returning partial optimistic results, as in when you toggle the item it won't show up after the optimistic update unless we add name and done in both mutations. We are currently checking out that bug.

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