Hi @mortenbekditlevsen -

sorry for closing this. We were expecting any in-flight PRs to automatically retarget to master once we merge combine-main.

Can you do this manually, and re-open the PR?

Thanks,
Peter

I reopened and changed the base branch to master. Make sure you resync the local branch.

I force pushed a commit that just contains the additions that were originally made on this branch. It had become too hard to clean up.

Latest commit addresses the latest batch of comments regarding doc comments and adds an overload of observe called valuePublisher as suggested.

Question: #6940 uses Result<Void, Error> for completions that can fail, but have no other parameters.

I initially modeled setValue in FirebaseDatabaseSwift similarly, but I was advised to change this to a completion of Error? instead.

Should we unify this behavior?

@peterfriese @ryanwilson

What are the advantages and disadvantages of the two different API styles?

The most important thing is of course the consistency, since the difference between the two is so small.

If I should argue for my favorite (Result<Void, Error>) I'd say that it carries slightly more semantic meaning than Error?.
In the former, the success path is the success case of a Result. In the latter it's the absence of an error.
And similarly, in the former the error value is bound to the failure case of the result - so it's very explicitly a failure and not just some 'random' value.

It's subtle and I'd say that most people would be ok with either choice. :-)

I also personally like the equality between:

a(completion: (Result<Void, Error>) -> Void)

a() -> Future<Void, Error>

a() async throws -> Void

(Even though the returned void in the last case would not be spelled out...)

@mortenbekditlevsen Given the minor differences, I'm fine going with your preference. If something compelling comes up in the future, we can change then.

@mortenbekditlevsen Given the minor differences, I'm fine going with your preference. If something compelling comes up in the future, we can change then.

SGTM as well, apologies for the delay in response.

@paulb777 Is this PR still relevant for the Swift Modernisation efforts?

It's marked as 'Changes requested', but I can't find any concrete suggestions that are not already addressed.

@mortenbekditlevsen Sorry for losing track of this PR. It looks like it needs to be rebased or merged with master. @peterfriese Do you see anything else to resolve before merging?

Combine is not in scope for the Swift Modernization first phase, but we'd like to continue working with the community on it.

@mortenbekditlevsen Sorry for losing track of this PR. It looks like it needs to be rebased or merged with master. @peterfriese Do you see anything else to resolve before merging?

I'd like to see some sample snippets added to FirebaseCombineSwift/README.md.

Also - if possible - it'd be great to add a sample screen to the SwiftUI example app at Example/CombineSample.

Morten - any chance you could add these? If you're pressed for time, we can merge this and add sample code later.

Hey @mortenbekditlevsen, I'm going to close this as it hasn't been discussed for >1.5 years and async/await and Observable seem like they're the future. Thanks for working through this and the discussion, let us know if you want us to reopen and start the conversation again.

I agree that that is a good idea. 😊 I could look into extensions for returning async sequences or an Observable model wrapper type. I’m experimenting with a Live generic wrapper around a Codable model entity that can be returned from RTDB or firestore.