extend CompositeDisposable with optional disposables sequence

[larryonoff]

What's in this pull request?

This PR extends CompositeDisposable that can be initialized with sequence of optional disposables.

[larryonoff]

I'm not sure that it's correct, but what about just replacing current version of init

init<S: SequenceType where S.Generator.Element == Disposable>(_ disposables: S)

with the following

public init<S: SequenceType where S.Generator.Element == Disposable?>(_ disposables: S) {
    var bag: Bag<Disposable> = Bag()

    for case let disposable? in disposables {
        bag.insert(disposable)
    }

    self.disposables = Atomic(bag)
}

[ikesyo]

Thanks for the pull request! It doesn't make sense to me, though. What is the benefit or concrete use case of this?

[larryonoff]

My business logic cases are backed by disposable in my case and view model in come case is composition of these disposables. Some of these business logic cases doesn't need to be backed by disposable.

As an example I have the protocol like the following:

protocol BehaviorType {
   func start() -> Disposable?
}

So as you see some behaviors don't have disposable to stop it (e.g. behavior that just sets text for UITextField).

ViewModel returns composition of these behaviors in the following way:

struct ViewModel {
  func activate() -> Disposable {
    return CompositeDisposable([
      userLoginBehavior().start(),
      newUserBehavior().start(),
      forgotPasswordBehavior().start()
    ])
  }
}

So by having allowing CompositeDisposable init with [Disposables?] I can write less and beautiful code.

Of course this is just a convenience, but this convenience doesn't break initial behavior and just extends initial behavior.

[RuiAAPeres]

I have two issues with this approach:

1. The design guidelines specifically state: prefer managing lifetime with operators over explicit disposal.
2. Your example is very imperative, which doesn't fit RAC style as well. If you don't care about the previous result and the error, you could chain them with a then.

[larryonoff]

I don't agree that this approach is very imperative.

Most of the examples that demonstrate UIKit + ReactiveCocoa have a standard state as a view model and this approach is very imperative, even that ReactiveCocoa is inside ViewModel.

The only state that I have is CompositeDisposable. ViewModel generally is just a function (Input) -> Disposable without any states. ViewModel is even inside a composition of functions (an order doesn't matter), where each function is a something that waits user input and begins execution only after user triggered an action.

My example is close to Signal.observe() -> Disposable.

There's no way to combine different business logic cases (e.g. create user, login) in one stream by just applying RFP operators working with UIKit. So we have to combine imperative and FRP approaches.

[mdiep]

Thank you for the pull request! ✨

Unfortunately, I don't think it makes sense to include this in ReactiveCocoa. Initializing a CompositeDisposable with a collection of Optional Disposables isn't a common operation and it's easily worked around by using flatMap like you've done in the definition here. But it sounds like it may be a useful extension in your project.

[larryonoff]

That's very strange for me. Because it just extends default init, it doesn't add any additional complexity to the library. This PR init can be uncommon for the RAC framework internals, but since library is used in many other projects nobody knows that it is only useful for my project and not other projects.

As for me it just sounds "we just don't like this PR, the reason doesn't matter".

[mdiep]

Because it just extends default init, it doesn't add any additional complexity to the library

It doesn't add complexity, but it does add code that needs to be maintained and supported. It also increases the size of the API, which can make things harder for users. This particular PR may seem like a simple addition, but these things do add up.

As for me it just sounds "we just don't like this PR, the reason doesn't matter".

I'm sorry, but that's just not true. I appreciate you going through the work to submit a PR. But we don't feel that this makes sense to include in the library based on (a) how common this need is, (b) how difficult it is for users if it's not there, and (c) how it increases the size of the API.

[larryonoff]

Thanks for discussing this with me. I was just rejected and ignored before.

This particular PR may seem like a simple addition, but these things do add up. It also increases the size of the API.

I completely understand. And I'm completely for the simple and stable API. This is why I suggested another way above (i.e. by just replacing current init with the new one). This doesn't increase the size of API. I've copied it below.

public init<S: SequenceType where S.Generator.Element == Disposable?>(_ disposables: S) {
    var bag: Bag<Disposable> = Bag()

    for case let disposable? in disposables {
        bag.insert(disposable)
    }

    self.disposables = Atomic(bag)
}

[larryonoff]

As an addition CompositeDisposable has method func addDisposable(d: Disposable?) which accepts Disposable?, so this's even more strange for me that init with Disposable? doesn't make sense.

[mdiep]

This is why I suggested another way above (i.e. by just replacing current init with the new one). This doesn't increase the size of API.

That doesn't increase the size, but it (1) will lead to errors if you try to pass in a [Disposable] and (2) is a breaking change with the current API.

As an addition CompositeDisposable has method func addDisposable(d: Disposable?) which accepts Disposable?

That's a much better argument! I'll reopen this to consider it some more.

[larryonoff]

(1) will lead to errors if you try to pass in a [Disposable] and (2) is a breaking change with the current API.

This's much better arguments for me.

As far as I understand, current PR code (not as suggested above) isn't a breaking change, but increases API a little.

Thanks for communication.

[mdiep]

This seems fine.