Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bind observable to collection of observers #166

Closed
wants to merge 7 commits into from

Conversation

sgtsquiggs
Copy link

@sgtsquiggs sgtsquiggs commented Jul 16, 2018

An extension on ObservableType to allow .bind(to:) with a collection of observers, and an extension on Collection (where Iterator.Element == Disposable) to allow .disposed(by:) with a collection of Disposables.

Fixes #163

@bobgodwinx
Copy link
Member

@sgtsquiggs This is a nice feature. I really like and already have something exactly like this in my codebase. It would be nice to add drive(in: ...) also what do you guys think? @freak4pc

@freak4pc
Copy link
Member

@bobgodwinx There was discussion in #163 where I also agreed it was a pretty great addition.

For drive(in:) I see way fewer use cases... Any examples?

@sgtsquiggs
Copy link
Author

I guess if you're manipulating UI controls (as in the example in #163) you may already be using Drivers to do so.

@freak4pc
Copy link
Member

In that case just drive(...) / drive([..]), IMO

@bobgodwinx
Copy link
Member

@freak4pc it's useful when you have more than one UI that changes on an event. So you just get the updates and drive(in: ...) at once. Another thing is discoverability while coding that's why I proposed drive(in: ..) so to make it stand out from the rest of the available methods. But drive(...) is also fine.

@freak4pc
Copy link
Member

freak4pc commented Jul 18, 2018

I think that if we're overloading bind(to:) with Collections, we should do the same for drive()... drive(in:) also isn't intuitive IMO

@bobgodwinx
Copy link
Member

Sounds like a deal let's go with drive(...) 👍🏿

@freak4pc
Copy link
Member

Cool. So, @sgtsquiggs would you like to take care of that?

Also, please look at the contribution guidelines: https://github.com/RxSwiftCommunity/RxSwiftExt/blob/master/CONTRIBUTING.md#submitting-a-pull-request

You need to add a playground page and update the README documenting these operators.

@sgtsquiggs
Copy link
Author

Hmm I can't seem to get the playground working. I'm not too familiar with Carthage, so may be that. It seems to be loading latest released RxSwiftExt from this repo instead of my local modified copy - so it does not have my new operator available. I had the same issue with adding tests; my test is the only one that is using @testable import RxSwiftExt.

Any pointers?

@freak4pc
Copy link
Member

@sgtsquiggs If you want to put your work on this branch, I can try and take a look

@freak4pc
Copy link
Member

@sgtsquiggs You forgot to make your extensions public ;-)
I also added a variadic option. Feel free to pull and proceed from here.

@sgtsquiggs
Copy link
Author

I seem to have broken the Playground with this last commit. It hangs before executing any code.

Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions. Also, it doesn't hang for me.

Readme.md Outdated
@@ -556,6 +557,28 @@ next((Test Class, 13))
completed
```

#### bindCollection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be bindMany like the other places.


let values = ["one", "two", "three", "four"]

let observers = [scheduler.createObserver(String.self),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make a test that confirms that All disposables are disposed ?

Copy link
Author

@sgtsquiggs sgtsquiggs Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do. I did not test because of CompositeDisposable's existing tests I wasn't aware CompositeDisposable was RxSwift and not RxSwiftExt 😑 .

@sgtsquiggs
Copy link
Author

Does this PR need any additional work?

@freak4pc
Copy link
Member

@sgtsquiggs Doesn't seem so. I want to give the discussion in ReactiveX/RxSwift#1702 a fair opportunity. Let's give it two more weeks or so, things move a bit slower there :)

@sgtsquiggs
Copy link
Author

In light of ReactiveX/RxSwift#1702 (comment) I think it would be best to use publish and connect instead of share.

@freak4pc
Copy link
Member

@sgtsquiggs There has been good movement on ReactiveX/RxSwift#1702 . Kruno suggests adding this only in RxSwift 5, but im not 100% sure since it doesn't introduce breakage. But he's open to adding it, so I suggest we keep following on Core.

@freak4pc
Copy link
Member

freak4pc commented Apr 9, 2019

This is coming in RxSwift 5. Closing. ReactiveX/RxSwift#1702

@freak4pc freak4pc closed this Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants