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

useCollection data gets populated twice when using multiple listeners (same collection, distinct queries) #1324

Open
bogdanciuca opened this issue Mar 14, 2023 · 2 comments · May be fixed by #1325
Labels

Comments

@bogdanciuca
Copy link

bogdanciuca commented Mar 14, 2023

Reproduction

https://stackblitz.com/edit/vuefire-usecollection-issue

Steps to reproduce the bug

Click on next / prev buttons to observe the behaviour.

Expected behavior

Snapshot data should be populated once, so things like pagination / load more can render smoothly.

Actual behavior

Snapshot data gets populated twice - once with incorrect results returned from the Firestore client cache (it uses the second listener's query) - and a second time with the expected results, pulled from the server. As you can see, this causes the item listing to flick and render twice.

Additional information

I kept the repro as simple as possible, so you may be wondering why am I using two listeners on the same collection. In the app I'm currently working on, I'm using useCollection inside Pinia stores and have paginated components (using queries similar to queryPaginator). I also have some dropdowns (using queries similar to queryFiltered), which I use to filter other collections. This pattern is quite consistent throughout the app, I am using it on 3 collections so far.

Proposed solution

Going back to the issue, since this is the intended behaviour of the Firestore client cache (as confirmed with the Firestore team, via support) and since vuefire doesn't expose the snapshot's metadata, I propose adding a new option to useCollection, something like ignoreCachedChanges. When set to true, the underlying ref won't be updated when results come from the cache, but rather wait for fresh results from the server. This would solve my use case and hopefully others. I will submit a PR shortly, let me know what you think. Thanks!

Copy link
Member

posva commented Mar 14, 2023

Thanks for looking into this, it's interesting. I did some digging and I think this requires something a bit more abstract than a new option ignoreCacheChanges but I'm not sure yet so I will take a deeper look once again, probably around June (to give you an honest estimate). In the meanwhile, using patch-package with your PR seems like a good compromise.

Notes to my future self:

  • This could maybe be solved with a custom debounced ref as the target
  • The computation of wait and pending is in this case not correct since the expectation of onSnapshot() triggering once for all the initial data is not true. I don't think there is a way to wait for the data to be ready like that unless we disable reads from the cache but I think those are global options

@bogdanciuca
Copy link
Author

bogdanciuca commented Mar 14, 2023

Thanks for looking into this, it's interesting.

That's the least I could do, thanks for all your work in the vuejs ecosystem.

This could maybe be solved with a custom debounced ref as the target

If you're referring to refDebounced, not sure how it can help, since the use case in network dependent.

I don't think there is a way to wait for the data to be ready like that unless we disable reads from the cache but I think those are global options

I'm still waiting on a response from the Firestore team on an option to disable cache reads, but I think it's unlikely, since their recommendation was to check fromCache and hasPendingWrites to ensure the data is coming from the server. In any case, I'll keep the issue updated.

PS: If you think of any alternate solution to fix this, let me know and I'll give it another try. If not, patch-package should work for now. Happy holidays!

LE: Looks like disabling cache reads is not an option, at least not in the short-term. 😞

@posva posva removed the bug label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants