CombineCommunity / CombineExt

CombineExt provides a collection of operators, publishers and utilities for Combine, that are not provided by Apple themselves, but are common in other Reactive Frameworks and standards.
https://combine.community
MIT License
1.73k stars 155 forks source link

Result of zip() on empty Collection is Empty() instead of Just([]) #84

Closed RustamG closed 3 years ago

RustamG commented 3 years ago

Thanks for your efforts on this set of useful features.

There's one thing that IMO should work slightly different. Although it would be a breaking change, I still wanted to bring attention to it.

The Collection.zip() operator returns Empty publisher in case the collection is empty (source). It means that the downstream methods like map, flatMap, etc. will not be called. And the receiveValue closure on sink() will not be called as well. My expectation is that those methods should still be called with an empty array passed. See the following example:

var cancelBag = Set<AnyCancellable>()
let publishers: [AnyPublisher<Int, Never>] = [
//    Just(1).eraseToAnyPublisher(),
//    Just(2).eraseToAnyPublisher()
]
publishers.zip()
    .map { results -> Int in
        // return the number of results. It could be a number of loaded items, etc.
        print("Map results")
        return results.count // will not be called :(
    }
    .sink { results in
        print("Got \(results)") // will not be called :(
    }
    .store(in: &cancelBag)

So, I'd changed

case 0:
    return Empty().eraseToAnyPublisher()

to

case 0:
    return Just([]).setFailureType(to: Element.Failure.self).eraseToAnyPublisher()

here

Please let me know your thoughts.

freak4pc commented 3 years ago

Thanks for opening this discussion @RustamG.

Unfortunately I believe the behavior you're suggesting is counter-intuitive. An array of no publishers isn't the same as a publisher of an empty array. The former is an empty stream and the later is a stream with a single value ([]).

It would be extremely confusing having an array that has no publishers (meaning no value) and the result of that being a single value being emitted (an empty array). Perhaps you have some business logic that depends on this, but the current behavior is the right one - and it's also in-line with how Reactive Extensions / RxSwift does this.

RustamG commented 3 years ago

Thanks @freak4pc. It totally makes sense. 👍