search

asm89 / Rx.PHP

Reactive extensions for PHP.
MIT License
205 stars 24 forks source link

Connectable Observables #16

Open davidwdan opened 8 years ago

davidwdan commented 8 years ago

I've implemented all of the connectable observables from RxJS. Since they're interdependent, it probably makes sense to handle them as one PR, but I can split them up if it's too much to review at once.

Before I create the PR, I need some feedback on the best way to handle the operators that can take a value or factory as the same argument. See:

https://github.com/davidwdan/Rx.PHP/blob/connectable_observables/lib/Rx/Observable/BaseObservable.php#L650 https://github.com/davidwdan/Rx.PHP/blob/connectable_observables/lib/Rx/Observable/BaseObservable.php#L697

Also, some of the tests are skipped because zip hasn't been implemented yet. @JMB3 is working on that.

asm89 commented 8 years ago

@davidwdan Tricky.

One option could be only implementing the factory versions, because the other version can be created using the factory one, but not the other way around. On the other hand some cases get really awkward if we do it that way I guess (e.g. publishValue(callable $...) vs publishValue($value)).

If we implement both version we could have a slightly different name for one of the versions?

davidwdan commented 8 years ago

I'm guessing that the vast majority of use cases in php will be without the factory. What if we only allow a subject for multicast and reverse the argument order for publishValue.

multicast(Subject $subject, callable $selector = null) publishValue($initialValue, callable $selector = null)

If someone wants to use a factory, they can still use MulticastObservable. It's not ideal, but I think it's better than the alternative.

If needed, we can always add a new multicast operator, down the road.

davidwdan commented 8 years ago

This ends up being much cleaner and you don't lose any functionality.

https://github.com/davidwdan/Rx.PHP/blob/connectable_observables_2/lib/Rx/Observable/BaseObservable.php#L650 https://github.com/davidwdan/Rx.PHP/blob/connectable_observables_2/lib/Rx/Observable/BaseObservable.php#L691

asm89 commented 8 years ago

@davidwdan I like it :+1: Let's figure out alternatives if someone comes by really wanting the factory support. :)

davidwdan commented 8 years ago

The only one that needs an alternative is multicast. How about:

multicastWithFactory(callable $subjectSelector, $selector = null)

asm89 commented 8 years ago

Should we just call $subjectSelector $subjectFactory if we go that route? I guess selector is more Rx-y..