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

[Bug?]: Orphaned Apollo Client on RedwoodApolloProvider re-render #11602

Open
1 task done
justinadkins opened this issue Sep 23, 2024 · 4 comments · May be fixed by #11699
Open
1 task done

[Bug?]: Orphaned Apollo Client on RedwoodApolloProvider re-render #11602

justinadkins opened this issue Sep 23, 2024 · 4 comments · May be fixed by #11699
Assignees
Labels
bug/needs-info More information is needed for reproduction topic/graphql topic/web

Comments

@justinadkins
Copy link

justinadkins commented Sep 23, 2024

What's not working?

With how the provider is currently constructed, a parent re-render will orphan an apollo client instance and create a new one. I believe this causes a memory leak and cache orphaning. I discovered it because of client pollution visible when using Apollo Client's dev tools.

This is due to the lack of memoization where the client is instantiated here.

How do we reproduce the bug?

Force a re-render of a parent of the RedwoodApolloProvider, for example App. Using the Apollo Client devtools, observe a new client instantiation each render.

Screenshot 2024-09-21 at 10 16 48 AM

What's your environment? (If it applies)

No response

Are you interested in working on this?

  • I'm interested in working on this
@Josh-Walker-GM
Copy link
Collaborator

Hey @justinadkins 👋

Thanks for the really clearly documented issue! I see you are interested in helping out with it, awesome! Are you hoping to create a PR with a fix? If so I'd be happy to help out in any way you might need.

@justinadkins
Copy link
Author

Hey @Josh-Walker-GM! Yes, I'm happy to submit a PR. I think the fix is pretty straight forward.

@callingmedic911
Copy link
Member

@justinadkins Are you thinking of using WeakRef to hold the the instance?

@justinadkins justinadkins linked a pull request Oct 17, 2024 that will close this issue
@justinadkins
Copy link
Author

@callingmedic911 Just seeing this now, I hadn't considered using WeakRef. I submitted a PR #11699 if you'd like to take a look. I'm jus using a local variable to keep track of the current instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/needs-info More information is needed for reproduction topic/graphql topic/web
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

3 participants