Making Single, Completable, Maybe all inherit from Observable

[LalitMaganti]

(or alternatively make all four inherit from a common base class)

At this point in RxJava 2.0 we have four NBP classes all of which have similar semantics but are currently totally decoupled from each other. Even though @akarnokd has been doing great work keeping all the code synced across the four operators this setup is a minefield for bugs and is extremely hard to follow the changes from an external watcher perspective (I don't know how you manage to review so many changes @JakeWharton!).

My proposal is to make the class have a some base class such that effort is not duplicated across different types.

@JakeWharton from another issue:

I have prototyped this and it requires at least two drastic changes:

• making operator methods non-final to allow overriding for usage of covariant return types to specialize in subclasses
• renaming of any combination operator name to avoid erasure collisions (e.g., flatMapObservable, flatMapSingle)

The huge upside is that you can treat any Single like an Observable which reduces the need for a lot of the specialized operators while still keeping only-once semantics where needed.

Obviously such a drastic change has both advantages and disadvantages.

Advantages:

1. (The huge one) Reduce duplication of efforts across classes.
2. Allow free downcasting to Observable for methods which only take them.

Disadvantages (from @akarnokd's post):

1. Many operations don't make sense outside the 0..N Observable
2. sounds like Reactor's Mono without backpressure
3. yes, you can reuse operator internals from Observable but lose the optimization potential knowing that only one of the onNext, onError or onComplete is ever called
4. won't work with the current architecture and would still require duplicate reactive base-class shells around: ObservableMap, SingleMap.
5. non-final methods may lead to megamorphic dispatch
6. no more onSuccess, has to call onNext followed by onComplete for Single and Maybe.

[LalitMaganti]

I'd like to address the disadvantages:

1. Correct. Which is why I would propose a common base class with the common operators and then you can have the Observable specific ones in their own class.
2. Not necessarily a bad thing (the part which is not liked in Mono is the backpressure right?)
3. I believe this to be an example of unnecessary micro-optimisation. With Maybe/Single/Completable, each method in the Observer is only called once. Do we really need to optimise which only happens once for a combinatorial explosion of operators?
4. A possibly valid point - I am still thinking about whether there is a way around this - I'd like to hear how @JakeWharton got around this in his prototype.
5. Conceeded
6. I'm not clear how this is the case? Surely you can just proxy the call through (i.e. listen for onNext/onComplete pair and call onSuccess after that)

[JakeWharton]

Not advocating or condemning this just yet, but just want to share some thoughts.

Many operations don't make sense outside the 0..N Observable

This is the single biggest problem I found. Thankfully I used Kotlin and could do @Deprecated(level=HIDDEN) which prevents them from being used (e.g., map on Completable). In Java this shows up just as a deprecated method when you use it on Completable which I didn't care about, but obviously would be the only approach here.

yes, you can reuse operator internals from Observable but lose the optimization potential knowing that only one of the onNext, onError or onComplete is ever called

In my prototype I still retained individual observers. If you subscribe to a Single with an Observer though you get wrapped so that onSuccess becomes onNext+onComplete.

won't work with the current architecture and would still require duplicate reactive base-class shells around: ObservableMap, SingleMap.

Non-goal. Nailing the consumer API is my only concern. This also further makes the above point not true since you can still optimize operators against singles, completables, etc.

non-final methods may lead to megamorphic dispatch

I'd be surprised if this was an actual problem in practice. Is the creation of the chain a hot path or is the execution of events down the chain the hot path? I would wager concerns of the latter outweigh the former 100x over.

no more onSuccess, has to call onNext followed by onComplete for Single and Maybe.

Same as above. You still get all the interface benefits except there's actually a lower conversion overhead.

[akarnokd]

When I first concieved Completable I implemented it in my own repo and once I was confident it is viable and working on its own, I posted a PR against RxJava.

Whatever unification you have in mind, try implementing a minimum viable "product" (in Java) and see how far the rabbit hole goes.

Thankfully I used Kotlin

Kotlin has extension methods so you could build a native RS library just by targeting interfaces. I did a similar thing in Reactor-Core.NET where many operators implement IFlux and IMono (both extending IPublisher) at the same time with the same runtime classes. From there IFlux<T> map(this IFlux<T>, Func<T, R>) and IMono<T> map(this IMono<T>, Func<T, R>) were just capturing the right type and returning the right type without conversion overhead.

[LalitMaganti]

I'd be happy to implement an MVP and see what falls out.

[JakeWharton]

Extension methods also get you invokestatic!

[akarnokd]

I'm closing this for now. I can imagine this done in Kotlin where there are extension methods and suppression and thus less painful than in Java.

[JakeWharton]

Here's my version from a few months ago which is incomplete:
https://github.com/JakeWharton/Reagent/

I didn't use extension methods but I'll have to try.

On Sat, Nov 12, 2016, 2:23 PM David Karnok notifications@github.com wrote:

Closed #4584 #4584.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4584 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEEZYsjGXaHKAavbCCMfH-ixReRMJqks5q9hIhgaJpZM4KET4U
.