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

Implement Query (part 1) and add improved support for Futures #21

Merged
merged 16 commits into from
Feb 15, 2024

Conversation

darinf
Copy link

@darinf darinf commented Feb 12, 2024

Adds support for Query.getDocuments. A follow-up PR will add support for the various other Query methods.

Most of this PR is about introducing new infrastructure for working with Firebase C++ Future instances:

  • Avoids needing to call Wait on the Future to block a thread for completion.
  • Just rely on the underlying OnCompletion machinery of Firebase that already does the right thing.
  • Add FutureProtocol and conformance through a new C++ subclass of Future<R> in the swift_cxx_shims namespace.
  • Enable sharing of code for Future handling.
  • Update DocumentReference.getDocument to use new Future handling.

A follow-up PR will revise setData to use new Future 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 these completion-callback variants, and we can just implement the async versions as a trivial layer around those.

Switches from @_spi(Error) to @_spi(FirebaseInternal) since we are exporting now more than just FirebaseError as SPI from FirebaseCore.

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 once setData is converted over to this new mechanism.

@darinf darinf changed the title Darin/query support Implement Query (part 1) and add improved support for Futures Feb 13, 2024
@darinf darinf marked this pull request as ready for review February 13, 2024 04:23
Copy link

@brianmichel brianmichel left a 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

Sources/FirebaseFirestore/Query+Swift.swift Show resolved Hide resolved
Sources/FirebaseCore/FutureProtocol.swift Outdated Show resolved Hide resolved
Sources/FirebaseCore/FutureProtocol.swift Show resolved Hide resolved
Copy link
Collaborator

@compnerd compnerd left a 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.

Sources/FirebaseAuth/FirebaseAuth+Swift.swift Outdated Show resolved Hide resolved
Sources/FirebaseCore/FutureProtocol.swift Outdated Show resolved Hide resolved
Sources/FirebaseCore/FutureProtocol.swift Show resolved Hide resolved
Sources/FirebaseCore/FutureProtocol.swift Outdated Show resolved Hide resolved
Sources/FirebaseCore/FutureProtocol.swift Outdated Show resolved Hide resolved
Sources/FirebaseFirestore/Query+Swift.swift Outdated Show resolved Hide resolved
Sources/FirebaseFirestore/Query+Swift.swift Outdated Show resolved Hide resolved
Sources/firebase/include/FirebaseCore.hh Outdated Show resolved Hide resolved
Sources/firebase/include/FirebaseCore.hh Outdated Show resolved Hide resolved
Sources/firebase/include/FirebaseCore.hh Outdated Show resolved Hide resolved
@darinf
Copy link
Author

darinf commented Feb 13, 2024

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.

@darinf
Copy link
Author

darinf commented Feb 13, 2024

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 completion callback API is actually built on top of the async API. That way the async API does not need to pay the cost of hoping thru the main thread.

@darinf darinf requested a review from compnerd February 13, 2024 18:10
Copy link
Collaborator

@compnerd compnerd left a 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.

@darinf darinf merged commit 4b93bed into main Feb 15, 2024
@darinf darinf deleted the darin/query-support branch February 15, 2024 00:08
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.

4 participants