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

Added (fixed) support for onSnapshot (issue 81) #158

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Venryx
Copy link

@Venryx Venryx commented Oct 24, 2019

The pull-request by BrianChapman (#130) mostly added support for onSnapshot, but had an issue where the onSnapshot callback sometimes wasn't called with the initial contents (if value was null at the time, or if called on a dec-ref instead of a query).

This pull-request is an updated version of his, fixing those issues.

EDIT: Apparently, BrianChapman made a newer pull-request (#145) with a fix for orderBy for it -- meaning we have two divergent pull-requests adding different fixes. If @soumak77 returns, one of us can integrate the other's fixes into a single onSnapshot merge. (too lazy atm since @soumak77 seems inactive currently)

BrianChapman and others added 5 commits January 23, 2019 08:38
…l the onSnapshot callbacks no mater where the flush is called from.
…tial callback-call.

First case where it would fail: the initial-data was null and it would think "no change", despite it needing to always trigger initially.

Second case where it would fail: calling onSnapshot on a document instead of a query.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants