-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement Query (part 1) and add improved support for Futures #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just had a few small questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to split this up into two separate PRs - one for the future changes and one for the FireStore Query additions.
Yeah, I don't think that is worth it. The point was to demonstrate the use of the new Future setup. I will have a follow-up that adds a lot more meat to the Query implementation. |
I did some more testing and found a substantial issue with the callbacks. Unfortunately the Firebase C++ SDK is not running callbacks on the main thread, which is in contrast to the Obj C API. I'll need to fix that. As a result I may invert the setup so that the |
45e08b4
to
7eb88a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clean ups with the protocol, it definitely feels much better.
Adds support for
Query.getDocuments
. A follow-up PR will add support for the various otherQuery
methods.Most of this PR is about introducing new infrastructure for working with Firebase C++
Future
instances:Wait
on theFuture
to block a thread for completion.OnCompletion
machinery of Firebase that already does the right thing.FutureProtocol
and conformance through a new C++ subclass ofFuture<R>
in theswift_cxx_shims
namespace.Future
handling.DocumentReference.getDocument
to use newFuture
handling.A follow-up PR will revise
setData
to use newFuture
handling as well.The
async
method variants are built on top of closure based APIs to better match the expectations of callers that are coded against the Obj C API. Arc needs thesecompletion
-callback variants, and we can just implement theasync
versions as a trivial layer around those.Switches from
@_spi(Error)
to@_spi(FirebaseInternal)
since we are exporting now more than justFirebaseError
as SPI fromFirebaseCore
.Also updates the
DocumentReference.get
to.getDocument
to match Firebase Swift API documentation and expectations of callers that are coded against the Obj C API.This approach makes the
OnCompletion_SwiftWorkaround
patch unnecessary. We can eliminate that oncesetData
is converted over to this new mechanism.