[Firestore] Add async counterpart for the addDocument method

[MojtabaHs]

The async version of the addDocument function exists only for Dictionary input, so I have added a codable version as well using the original existing methods.

• The function signature is from the original; I just converted the completion parameter to async.
• The documentation is from the original; I only added the Throws line from another existing function.
• The implementation is from another existing function, but it uses the original addDocument which takes an encodable.

[paulb777]

Thanks @MojtabaHs! I'd rather see this added to Firestore/Swift/Source/Codable/CollectionReference+WriteEncodable.swift since that file is already Swift and it would be closer to the non-async implementation.

@ncooke3 @wu-hui Do you know if it was an oversight that this is missing or some other reason?

[paulb777]

This catch looks redundant.

[MojtabaHs]

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?

[paulb777]

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.

[MojtabaHs]

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.

[MojtabaHs]

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.

[peterfriese]

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

[MojtabaHs]

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.

[paulb777]

@MojtabaHs Thanks for the PR. Given the open questions we're seeing, we want to a deeper review of the Firestore APIs and make sure we're being consistent with how we handle async and throwing. It may take us a while longer to move forward here.

[MojtabaHs]

@paulb777 Let me know if there is anything I can do to help this progress.

[MojtabaHs]

Any updates on this? There are other opportunities to introduce async counterparts to modernize the API, but I’m concerned that this decision could affect those efforts as well. I believe it’s important to address this first.

[ncooke3]

Hi @MojtabaHs, no updates as of yet. This is something actively being looked into. I'll update when the direction is more clear.

[MojtabaHs]

It's been a while, how can I assist you in moving forward with this progress?