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

2.x: switchMapSingle and switchMapCompletable #4853

Closed
tomaszpolanski opened this issue Nov 15, 2016 · 32 comments
Closed

2.x: switchMapSingle and switchMapCompletable #4853

tomaszpolanski opened this issue Nov 15, 2016 · 32 comments

Comments

@tomaszpolanski
Copy link

tomaszpolanski commented Nov 15, 2016

Hey,

the addition of flatMapSingle/flatMapCompletable made the usage of Single and Completable way nicer in RxJava 2.
Do you consider adding switchMapSingle/switchMapCompletable to RxJava 2 as well?

Cheers

@tomaszpolanski tomaszpolanski changed the title switchMapSingle and switchMapCompletable 2.x: switchMapSingle and switchMapCompletable Nov 15, 2016
@akarnokd
Copy link
Member

You mean 1.x? I have no plans for that and I don't want to keep 1.x alive for too long. I'd like to stop enhancing 1.x in 6 months and enter it into a bugfix-only mode. Otherwise 3rd party libraries may delay their upgrade way longer.

@vanniktech
Copy link
Collaborator

He wants switchMapSingle and switchMapCompletable for RxJava 2 just like there is flatMapSingle and flatMapCompletable

@tomaszpolanski
Copy link
Author

@vanniktech exactly

@akarnokd
Copy link
Member

Ah right. No plans for extending any other xMap operator

@vanniktech
Copy link
Collaborator

But then concatMap also needs the variants in order to be consistent. To be honest I forgot about those when proposing flatMapSingle / flatMapCompletable / flatMapMaybe initially - #4667

And then there's also Maybe. Plus Observable and Flowable are able of mapping so it's easily another 12 methods.

  • Observable.switchMapSingle
  • Observable.switchMapMaybe
  • Observable.switchMapCompletable
  • Observable.concatMapSingle
  • Observable.concatMapMaybe
  • Observable.concatMapCompletable
  • Flowable.switchMapSingle
  • Flowable.switchMapMaybe
  • Flowable.switchMapCompletable
  • Flowable.concatMapSingle
  • Flowable.concatMapMaybe
  • Flowable.concatMapCompletable

@tomaszpolanski
Copy link
Author

If it is not on the roadmap because you don't want to add any more xMap operators, then I understand.

But if it is due to lack of the time, would you consider accepting contributions on the topic?

@akarnokd
Copy link
Member

Sure, but these operators are not easy.

@tomaszpolanski
Copy link
Author

Yes, I know, those are one of the hardest.
Still, I will at least have a look and see if I can deliver a PR.

Cheers!

@akarnokd
Copy link
Member

Any progress on this?

@davidmoten
Copy link
Collaborator

I'm happy to have a look at this one if @tomaszpolanski doesn't have time at the moment.

@tomaszpolanski
Copy link
Author

@davidmoten It would be great! My available time now is pretty limited.

@davidmoten
Copy link
Collaborator

So the list of new operators is quite big. Currently:

  • Observable.switchMapSingle
  • Observable.switchMapMaybe
  • Observable.switchMapCompletable
  • Observable.concatMapSingle
  • Observable.concatMapMaybe
  • Observable.concatMapCompletable
  • Flowable.switchMapSingle
  • Flowable.switchMapMaybe
  • Flowable.switchMapCompletable
  • Flowable.concatMapSingle
  • Flowable.concatMapMaybe
  • Flowable.concatMapCompletable

I imagine we can add support for all of these operators straight away using composition and write dedicated operators later one by one. An example would be Observable.switchMapSingle:

public final <R> Observable<R> switchMapSingle(
    final Function<? super T, ? extends SingleSource<? extends R>> mapper) {
        Function<? super T, Observable<R>> mapper2 = t -> 
            {
                SingleSource<? extends R> source = mapper.apply(t);
                Single<? extends R> single;
                if (source instanceof Single) {
                    single = (Single<? extends R>) source;
                } else {
                    single = Single.unsafeCreate(source);
                }
               return (Observable<R>) single.toObservable();
            };
        return switchMap(mapper2, bufferSize());
    }

This is just to demo the idea, I wouldn't use an anonymous class and lambdas are just used for readability.

@akarnokd any interest in this?

@akarnokd
Copy link
Member

I imagine we can add support for all of these operators straight away using composition and write dedicated operators later one by one.

Indeed.

An example would be

If you do this, there is an unwritten guideline that the main base types should have no (anonymous) inner classes. It helped me a lot before not having to deal with Flowable$1$2 and similar entries. The go-to place for these are in io.reactivex.internal.operators.flowable.FlowableInternalHelper and its respective variants.

any interest in this?

I'm more interested in the direct implementations but at least the overall usefullness of the API extension could be vetted.

@davidmoten
Copy link
Collaborator

If you do this, there is an unwritten guideline that the main base types should have no (anonymous) inner classes. It helped me a lot before not having to deal with Flowable$1$2 and similar entries. The go-to place for these are in io.reactivex.internal.operators.flowable.FlowableInternalHelper and its respective variants.

Yep no problems. I've appreciated consistent naming of non-anon classes inside operators for debugging purposes and code searches too.

I'm more interested in the direct implementations but at least the overall usefullness of the API extension could be vetted.

Sure. I'll proceed with PRs with unit tests for the new operators and I'm happy to have a stab at dedicated operators after that.

@marcinsus
Copy link

What is expected behaviour for Observable.switchMapMaybe and Observable.switchMapCompletable? I can try to take care of it but I am not convinced what that method should return.

@feresr
Copy link

feresr commented Aug 2, 2017

In the mean time, would it be correct to assume that an acceptable workaround for this is as follows?: .toObservable()

upstream.concatMap {
        someObservable().collect{...}.toObservable()
    }

@marcinsus
Copy link

marcinsus commented Aug 3, 2017

@feresr
Probably yes, at this moment in few places inside my code i have this type of stucture. It is very weird and uncessesery code but it is understandable.
@akarnokd
What do you think about method switchMapCompletable which emit Object or maybe some enum? On the other hand we can we can swallow information about stream and do not emit anything.

@akarnokd
Copy link
Member

akarnokd commented Aug 3, 2017

@marcinsus I don't fully understand your question. Use switchMapCompletable().andThen(Single) to emit something after the switch completed.

@PaulWoitaschek
Copy link
Contributor

The problem he talks about is that switchMapCompletable does not exist.

switchMapSingle returns an Observable of the Single type.

So technically switchMapCompletable should return Void. (Which it can't, because that has no instance.) So he asks if the signature should rather expose some enum Nothing or just an Object

@JakeWharton
Copy link
Contributor

An Observable.switchMapCompletable should return Completable.

@PaulWoitaschek
Copy link
Contributor

You are right as flatMapCompletable returns a Completable.

But now I find it weird that
Observable.flatMapSingle<T>returns an Observable<T>
but
Observable.flatMapCompletable returns an Completable.

@JakeWharton
Copy link
Contributor

A stream of 5 elements flatMap'd to a stream of a single element still results in a stream of 5 elements.

@dweebo
Copy link
Contributor

dweebo commented Oct 6, 2017

I'm working on implementing concatMapCompletable across the reactive types.

I'm wondering if the library should also have implementations of
concatMapDelayErrorCompletable, concatMapEagerCompletable, concatMapEagerDelayErrorCompletable?

@akarnokd
Copy link
Member

akarnokd commented Oct 6, 2017

concatMapEagerCompletable is indistinguishable from flatMapCompletable because all sources run concurrently and their only output is a terminal event; no items have to be buffered until their turn. Also the maxConcurrency == 1 gets you concatMapCompletable behavior.

@dweebo
Copy link
Contributor

dweebo commented Oct 6, 2017

Ahh yes that makes sense.

So is there any reason to implement concatMapCompletable at all? If so do you want it to just use flatMapCompletable() internally or should there be a new direct implementation (what I've been working on)?

@akarnokd
Copy link
Member

akarnokd commented Oct 6, 2017

Actually, Observable.flatMapCompletable doesn't have maxConcurrency overload thus there might be a reason to implement concatMapCompletable.

@dweebo
Copy link
Contributor

dweebo commented Oct 6, 2017

Ok I'll keep at it then

@eygraber
Copy link

eygraber commented Nov 1, 2017

#4853 (comment)

Did anything ever come of this?

@akarnokd
Copy link
Member

akarnokd commented Nov 1, 2017

#5161 and #5649 added some operators.

@eygraber
Copy link

eygraber commented Nov 1, 2017

So there are plans to implement the other operators, but not in any particular timeframe?

@akarnokd
Copy link
Member

akarnokd commented Nov 1, 2017

The listed operators can simply expressed with the existing default operators and toObservable/toFlowable in the lambda parameters. Unfortunately, very few people could or are willing to write them in an optimized and inlined fashion - which would add more value to the library than a simple lambda-rewriting function. Imagine, you have to write a proper javadoc with a marble diargram and several hundred lines of unit tests to validate and cover a new operator with some 3 lines of code. Kotlin's extension methods really shine in allowing interested parties to define such convenience functions locally to their project and not boomeranging such features through the RxJava library. The issue is left open in case somebody really wants to take the time and effort to contribute but as the core developer, I'm not particularly interested myself in tackling these types of operators.

@akarnokd
Copy link
Member

akarnokd commented Mar 7, 2018

Closing via #5870, #5871, #5872, #5873, #5875.

@akarnokd akarnokd closed this as completed Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants