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

broadcastClient: removing query from one tab removes it from all tabs, even if others still have observers #7487

Open
c-ehrlich opened this issue May 28, 2024 · 2 comments
Labels
bug Something isn't working help wanted Extra attention is needed package: broadcast-client

Comments

@c-ehrlich
Copy link
Contributor

c-ehrlich commented May 28, 2024

Describe the bug

When using broadcastQueryClient:

  • Unmounting a query (with the last observer being removed) in one tab also removes that query in the other tab after gcTime hits
  • After re-mounting the query, it is still not observed by the other tab, and invalidation works in neither direction

Your minimal, reproducible example

(no stackblitz as their containerization seems to break broadcast channels, but see "Steps to reproduce" for a repo, also I've recorded a video demo)

Steps to reproduce

Sample repo: https://github.com/c-ehrlich/broadcast-invalidation-demo

To see the bug

  • cd before, pnpm i, pnpm dev
  • open the application in two windows, show devtools
  • invalidating across windows should work
  • click "hide" in one window, wait for gcTime - the query is now no longer observed in both windows, and invalidation is broken
  • click "show" to get the query back - invalidation will still be broken in both directions

To see a demo of my rough fix

Expected behavior

  • Unmounting a query (with the last observer being removed) in one tab should not remove that query in the other tab
  • Invalidation should work across tabs even when a query is not being observed in the tab that invalidates it

(not sure what the intended behaviour is, but these would be my expectations for the broadcastClient)

How often does this bug happen?

Every time

Screenshots or Videos

https://www.loom.com/share/e7da8b5d3cdb4b36a3fbf076d7ccb6b8

(the data updates twice shortly after each other at 1:16 because i had a third tab open, see "Additional context")

Platform

Chrome 124.0.6367.208 on MacOS 14.3 (also confirmed in Firefox on Windows 11)

Tanstack Query adapter

react-query

TanStack Query version

5.39.0

TypeScript version

5.2.2

Additional context

My implementation still has one significant inefficiency:

  • open three tabs instead of two
  • hide the query in one of them, and invalidate there. now both of the other windows will refetch
  • ideally, only one refetch would be necessary. i could imagine achieving this through either "borrowing" the query from an instance that has it, or by keeping track of invalidation across instances and making only one of them refetch

Other considerations:

  • @TkDodo mentioned in this discussion that "the broadcastQueryClient plugin just assumes the same queryClients in all tabs". Not quite sure what this means. If it were possible to share an actual QueryClient, would it not become unnecessary to pass messages between tabs? But if that is somehow possible, it would be a much better solution.
  • I would be open to work on this if you agree on the intended behaviour
@TkDodo
Copy link
Collaborator

TkDodo commented Jun 10, 2024

Thanks for the reproduction. I think the issue is mostly that we are listening to updated and removed events, but not added events. So after a query has been removed, we sync that, but if we add a new Query, we don't. Wouldn't the fix be to simply listen to the added event somewhere here and then broadcast that, too?

if (queryEvent.type === 'updated' && queryEvent.action.type === 'success') {
channel.postMessage({
type: 'updated',
queryHash,
queryKey,
state,
})
}
if (queryEvent.type === 'removed') {
channel.postMessage({
type: 'removed',
queryHash,
queryKey,
})
}
})

I don't fully understand your fix around if ((queryEvent.type as string) === "invalidated") { because invalidated is not a valid queryEvent:

export type QueryCacheNotifyEvent =
| NotifyEventQueryAdded
| NotifyEventQueryRemoved
| NotifyEventQueryUpdated
| NotifyEventQueryObserverAdded
| NotifyEventQueryObserverRemoved
| NotifyEventQueryObserverResultsUpdated
| NotifyEventQueryObserverOptionsUpdated

@TkDodo TkDodo added bug Something isn't working help wanted Extra attention is needed package: broadcast-client labels Jun 10, 2024
@c-ehrlich
Copy link
Contributor Author

c-ehrlich commented Jun 11, 2024

i will try the added event solution tomorrow :)

the invalidated event is something that i patched into the QueryClient, see: c-ehrlich/broadcast-invalidation-demo@e2133b9#diff-b432175c3da3a30468adb42601d364ab07e49f55f25a72d0da521a87cb968423R28

(obviously not the correct implementation, just a demo)

edit: tried adding the added event that runs queryCache.build when received, still same behaviour. i think for this to work, added would need to add a fake QueryObserver to the query in the tab receiving the message, and removed would need to remove that queryObserver again, and additionally each new tab would need to know the present state of the other tabs, not just receive new messages from that point onward. seems difficult to implement with the current architecture - the fundamental problem is that tabs are sharing the state of the QueryClient, but not their QueryObservers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed package: broadcast-client
Projects
None yet
Development

No branches or pull requests

2 participants