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.

Sign up for GitHub

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

Jump to bottom

[Firestore] Add async counterpart for the addDocument method #13407

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[Firestore] Add async counterpart for the addDocument method #13407

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7ab2434
feat: add the async counterpart of the `addDocument` function with th…
MojtabaHs Jul 26, 2024
c27db06
doc: add throws part
MojtabaHs Jul 26, 2024
87e080b
doc: enhance doc formatting
MojtabaHs Jul 26, 2024
63a80d5
style: reformat a line to satisfy the linter line style check
MojtabaHs Jul 26, 2024
5bd75f2
style: move the function closer to the non-async implementation
MojtabaHs Jul 27, 2024
6005fe9
feat: add availability to support concurrency
MojtabaHs Jul 27, 2024
af4e2b8
feat: catch the throwing error and resume it to the continuation
MojtabaHs Jul 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions Firestore/Swift/Source/Codable/CollectionReference+WriteEncodable.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,36 @@ public extension CollectionReference {
let encoded = try encoder.encode(value)
return addDocument(data: encoded, completion: completion)
}

/// Encodes an instance of `Encodable` and adds a new document to this collection
/// with the encoded data, assigning it a document ID automatically.
///
/// See `Firestore.Encoder` for more details about the encoding process.
///
/// - Parameters:
/// - value: An instance of `Encodable` to be encoded to a document.
/// - encoder: An encoder instance to use to run the encoding.
/// - Throws: `Error` if the backend rejected the write.
/// - Returns: A `DocumentReference` pointing to the newly created document.
@available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *)
@discardableResult
func addDocument<T: Encodable>(from value: T,
encoder: Firestore.Encoder = Firestore.Encoder()) async throws
-> DocumentReference {
return try await withCheckedThrowingContinuation { continuation in
var document: DocumentReference?
do {
document = try self.addDocument(from: value, encoder: encoder) { error in
if let error {
continuation.resume(throwing: error)
} else {
// Our callbacks guarantee that we either return an error or a document.
continuation.resume(returning: document!)
}
}
} catch {
Copy link
Member

Choose a reason for hiding this comment

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

This catch looks redundant.

Copy link
Contributor Author

@MojtabaHs MojtabaHs Jul 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It catches the possible thrown error by calling the original addDocument function (which seems only throwable by the underlying encoder). Since withCheckedThrowingContinuation takes a non-throwing closure, we can't just rethrow it.

Am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. It looks like the problem is with the existing API above that both throws and does a callback with an error parameter.

Copy link
Contributor Author

@MojtabaHs MojtabaHs Jul 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agree, I was about to refactor it but stopped because it is part of the public API and would be backward-incompatible.

By the way, it's not clear to me what to return when encountering encoding errors.

I can open another PR for refactoring the existing API and discuss it further separately. Let me know your opinion.

Copy link
Contributor Author

@MojtabaHs MojtabaHs Jul 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

About this PR, how about moving this function back to the CollectionReference+AsyncAwait and using the existing API there like:

func addDocument<T: Encodable>(from value: T,
                               encoder: Firestore.Encoder = Firestore.Encoder()) async throws
-> DocumentReference {
  let encoded = try encoder.encode(value)
  return try await self.addDocument(data: encoded)
}

So no need for withCheckedThrowingContinuation at all.

Why move this to the other file? Because otherwise, this extension would depend on another extension.

Let me know your opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #9157 (comment) for a discussion about an edge case when the client is offline. This would result in the async method never returning.

Copy link
Contributor Author

@MojtabaHs MojtabaHs Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Using this method will depend on the existing async API. So, at the end of the day, we should either deprecate both of them or agree on the current existing logic.

continuation.resume(throwing: error)
}
}
}
}
Loading