firebase / firebase-ios-sdk

Firebase SDK for Apple App Development
https://firebase.google.com
Apache License 2.0
5.55k stars 1.45k forks source link

Async/Await missing for setData #9157

Open Joebayld opened 2 years ago

Joebayld commented 2 years ago

[REQUIRED] Step 1: Describe your environment

[REQUIRED] Step 2: Describe the problem

When using the new async/await methods in swift, they seem to be missing when setting an Encodable object to the Firestore.

Steps to reproduce:

Try the below code using the library. It will throw these warnings: No 'async' operations occur within 'await' expression No calls to throwing functions occur within 'try' expression

func setFriend() async throws { 
    try await db.collection("friends").document("123").setData(from: friend, merge: true)
}

That's because it's not seeing an async version of the function.

But, if you were to try:

func setFriend() async throws { 
    try await db.collection("friends").document("123").setData(["some": "data"], merge: true)
}

This works because async/await is available for that function.

My possible solution

I created this extension that seems to solve it. If we all agree, could this go in the main library? That would give us full support for setting data via async/await.

extension FirebaseFirestore.DocumentReference {
    func setData<T: Encodable>(from: T, merge: Bool = false) async throws {
        let encoder = Firestore.Encoder()
        let data = try encoder.encode(from)
        try await setData(data, merge: merge)
    }
}
mortenbekditlevsen commented 2 years ago

I agree with you, that such an API should exist in the main library.

I do see a big potential issue, which is not really something new, but that I think could be very, very unexpected in the async/await world.

The completion parameter of setData will not be called while the client is offline. This is of course existing behavior and as such your API is the 'correct' translation. But I'm pretty sure that I read that async functions are expected to complete (which this theoretically also does if and when the client comes online, but depending on the environment, it may not).

I am uncertain of the best API, but I think that adding a timeout parameter with some appropriate default value, and then let the function throw in case the timeout occurs before the setData completion is called.

@schmidt-sebastian @paulb777 @peterfriese @ryanwilson

Joebayld commented 2 years ago

I didn’t even consider that. Hmm..

I wonder if maybe when using the async/await features that it must be online? Or would that be a bad requirement.

In my specific app, I don’t want these actions to commit if I’m offline. My solution has been wrapping it into a transaction. I’m curious if maybe async functions have different behaviors or if there’s an option that can be set to force to the server.

schmidt-sebastian commented 2 years ago

I will discuss with my team, but I don't see an obvious way to address this problem. The described behavior already exists for all automatically "generated" async/await methods. While we can add a timeout, this would essentially mean that we would have to fail the actual write (and not retry at a later point) and as such, I would see this as an optional and possibly somewhat orthogonal feature request.

Joebayld commented 2 years ago

What would be the preferred way to force making sure the document is written to the server then? A transaction?

For example - on sign out I want to force a document to save before signing the user out. But I'm not sure when that would happen if the user is offline. I feel like that's a scenario where async/await is useful. I'm essentially trying to make the Swift API behave like the javascript API when you just await.

schmidt-sebastian commented 2 years ago

The "async/await" API will behave the same way as the callback API. It will resolve once the document has been written - and potentially block forever if the client is offline. If you don't want this blocking behavior you can indeed use a transaction, which will try to write the document but fail when offline. Note that it might still take a minute for the transaction to fail as we will retry 5 times with backoff.

alexfringes commented 2 years ago

It feels like what I would actually want is for the offline state to return an error containing an AsyncSequence of "OfflineWriteResolution" states ("online again", "reattempting write", "succeeded", "failed" etc.). This keeps the happy path concise by isolating anything special I may want do in an offline scenario and letting me do so in an async/await manner. Presumably this is also closest to providing the same nuance as the completion handler does in this case in the callback world?

Mr-Goldberg commented 1 year ago

This feature is already implemented for firebase-js-sdk (not sure I named it correctly). I don't really see the difference between "completion" which's never called and part of a function after "await" which was suspended forever. Probably the solution with the transaction will work. If you can, please proceed with implementing this feature, possibly with solution proposed by @Joebayld.

schmidt-sebastian commented 1 year ago

@ehsannas

krris commented 8 months ago

Hi 👋 Any updates on this?