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 runTransaction and batch support #28

Merged
merged 14 commits into from
Feb 16, 2024
Merged

Conversation

darinf
Copy link

@darinf darinf commented Feb 15, 2024

Adds:

  • Transaction, TransactionOptions and WriteBatch types
  • VoidFuture as workaround for Future<void>
  • TransactionWeakReference as a wrapper for Transaction since Transaction cannot be exposed directly to Swift given that it is a non-copyable type.

@darinf darinf marked this pull request as ready for review February 15, 2024 22:17
@darinf darinf requested review from fiedukow and hyp February 15, 2024 22:17
@darinf darinf changed the title Darin/transaction support Implement runTransaction and batch support Feb 15, 2024
@@ -34,6 +34,63 @@ extension Firestore {
public func collection(_ collectionPath: String) -> CollectionReference {
swift_firebase.swift_cxx_shims.firebase.firestore.firestore_collection(self, std.string(collectionPath))
}

public func runTransaction(_ updateBlock: @escaping (Transaction, UnsafePointer<NSError?>?) -> Any?, completion: @escaping (Any?, Error?) -> Void) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this need changes from https://github.com/thebrowsercompany/swift-firebase/pull/30/files for the error pointer?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that PR builds on top of this one. I wrote that one once I was able to try building against Arc. Rather than complicate things by folding it into this PR, I just left them as separate. Hope that works!

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.

This looks pretty good to me, had some questions around who maintains the reference to future, but otherwise LGTM

@darinf darinf merged commit e19b8fa into main Feb 16, 2024
1 check passed
@darinf darinf deleted the darin/transaction-support branch February 16, 2024 16:45
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