-
Notifications
You must be signed in to change notification settings - Fork 24
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
Connectable Observables #16
Comments
@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. If we implement both version we could have a slightly different name for one of the versions? |
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
If someone wants to use a factory, they can still use If needed, we can always add a new multicast operator, down the road. |
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 |
@davidwdan I like it 👍 Let's figure out alternatives if someone comes by really wanting the factory support. :) |
The only one that needs an alternative is multicast. How about:
|
Should we just call |
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.The text was updated successfully, but these errors were encountered: