Allow binding to multiple observers

[freak4pc]

This is based on a PR in RxSwiftExt: RxSwiftCommunity/RxSwiftExt#166

As well as a discussion here: RxSwiftCommunity/RxSwiftExt#168

I wondered if you'd be open to adding something like this to the main repo as it seems useful, and would also save us from a few external overloads, possibly.

It's also possible having separate overloads for bind(to: Observer), bind(to: Observer...), bind(to: [Observer]) but I don't think there would be a significant performance difference in that case. (edit: Running the Benchmarks tests confirms that)

If this makes sense to you, I'll add everything needed including tests etc. This maintains backward compatibility.

[RxPullRequestBot]

	2 Warnings
⚠️	Please target PRs to develop branch
⚠️	No CHANGELOG changes made

Generated by 🚫 Danger

[pedrommcarrasco]

I'm in for variadic parameters and if the benchmarks prove that the overload impact isn't significant I guess it's fine :)
👍 and ❤️this initiative!

[kzaher]

Hi @freak4pc ,

I think that adding these extensions to generic observer is dangerous. The name of this thing is bind(to:, and it doesn't mention anything about creating additional share intermediate step.

This unfortunately can't cover all of the cases properly and creates a lot of confusion.

E.g. If you use share, this will fail in case you are binding stateful entity (that emits first value immediately) because other observers won't receive the initial value.

If you shareReplay then it will probably work, but it's weird, and I'm sure there will be some other unwanted side effect.

I think it might sense to have this for Driver or Signal.

It might make sense to hold these kinds of changes from single to variadic until next major version since it is public interface breakage.

[freak4pc]

E.g. If you use share, this will fail in case you are binding stateful entity (that emits first value immediately) because other observers won't receive the initial value.

If that is the case, there should be a test case to cover it IMO - My impression was this is fine since the entire test suite passed.

Anyways I think binding to many has the underlying meaning of using a shared resource. I don't mind having an array-only variation, without variadic, and I think that works well since it fits both scenarios.

Regarding stateful, yeah you're right this would probably need a share(replay: 1) instead.

[kzaher]

If that is the case, there should be a test case to cover it IMO - My impression was this is fine since the entire test suite passed.

The fact that test case passes isn't the evidence that the code works, it only means that there is the lack of evidence that it doesn't work :)

Ok, I've checked out the code again. If you change the implementation to be

public func bind<O: ObserverType>(to observers: O...) -> Disposable where O.E == E {
    return self.subscribe { event in
        observers.map { $0(event) }
    }
}

and ditch the array version then you don't have any multiple subscription issues, we don't add any additional methods and just improve the existing one.

It still breaks the public interface, so if we want to pull this in, just to be safe, maybe we should wait until RxSwift 5.0.

[freak4pc]

@kzaher

You mean $0.on(event), not $0(event) - correct? Also we'll need to add a _ = before the map since the result isn't actually used, in which case, perhaps better to use forEach ?

[freak4pc]

@kzaher Made the changes required, let me know if makes sense for you.

I had to add array overloads, since Arrays don't support splatting yet (so it wouldn't be able to reuse the Optional vs. non-Optional uses). I made them private since you wanted to avoid Array-style bindings, but TBH I think it would be better to make them public and have both options open. LMK your thoughts on this.

I also extended this to Relays, but I just noticed - could it be there are no tests for any kind of relays at this point?

P.S Even though there is an interface change - in practice the end-user interface doesn't change, since they can still bind to a single ObserverType with no breakage / code changes. For that reason I don't see a necessity to wait for RxSwift 5 necessarily, unless you're worried and prefer to roll this out later on.

[kzaher]

I also extended this to Relays, but I just noticed - could it be there are no tests for any kind of relays at this point?

I believe they are below your code.

[freak4pc]

Oh, sorry, duh. Blacked out there :)

[freak4pc]

@kzaher Added the tests, ready for you

[freak4pc]

Push passed but PR didn't - not sure if related 🤔
Thoughts @kzaher ?

[kzaher]

This is not testing the variadic case, is there a need for this test and the non variadic ones?

[freak4pc]

As written below, in my though process, yes -
I believe our tests should always test that the existing interface and the old interface works (e.g. binding to a single one vs. binding to a variadic list).

[freak4pc]

I've addressed your feedback @kzaher.

I believe we should test the single and variadic tests separately to ensure reliability and backward-compatibility. Let me know if you'd like this to be done otherwise.

[freak4pc]

BTW lets merge #1924 first, would make it easier to rebase this.

[freak4pc]

As expected this is conflicted. Taking care of this RN.

[freak4pc]

Ok, I redid the work on top of the RxRelay work. This should be good to go as well.

[freak4pc]

@kzaher CI is green whenever you're ready to give this another review.