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

Issue 81 onsnapshot #130

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

Conversation

BrianChapman
Copy link

@BrianChapman BrianChapman commented Jan 27, 2019

Implements onSnapshot. See #81

@derekparker
Copy link

derekparker commented Feb 7, 2019

This is great, I'm glad I found this because I really needed this functionality in tests, however in it's current state I don't think it'll quite work yet.

The functionality missing (or not quite working) is that:

  1. the first time calling onShapshot it should return the current state of the document / collection, not just when something changes.
  2. When chaining this off of a .where(...) it doesn't seem to work at all.

I think 2 is more related to a fundamental issue with this mocking library in that where() seems to resolve immediately, whereas in the real library it returns a lazy ref that must be resolved with get().

@BrianChapman
Copy link
Author

@derekparker I updated this to return both immediately and when something changes. I'm not sure then where operator is fully implemented.

@Venryx
Copy link

Venryx commented Oct 23, 2019

These changes work well in my project.

Is there a reason this PR hasn't been able to be merged yet?

EDIT: Nevermind, I did notice an issue with the code: 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).

Created a new pull-request which resolves those issues: #158

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.

None yet

3 participants