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

feat(operator): add concurrency parameter to switchMap #215

Closed
staltz opened this issue Aug 27, 2015 · 17 comments
Closed

feat(operator): add concurrency parameter to switchMap #215

staltz opened this issue Aug 27, 2015 · 17 comments

Comments

@staltz
Copy link
Member

staltz commented Aug 27, 2015

This issue is just a heads-up about an operator that could be added to RxJS Next. I can also see this used through lift instead of bundling it straight in the library.

Basically flatMapLatestWithMaxConcurrency:

http://stackoverflow.com/questions/32233408/how-to-implement-rxjs-flatmaplatesttwo

What do you think?

@benlesh
Copy link
Member

benlesh commented Aug 27, 2015

Hmm... I think flatMap and flatMap latest should support concurrency limits regardless. Considering they're basically sugar for map(x => obs).mergeAll() and merge definitely supports concurrency limits.

@staltz
Copy link
Member Author

staltz commented Aug 27, 2015

Oh cool! I didn't know mergeAll() had a concurrency param. It doesn't in Reactive-Extensions/RxJS. That's great.

@benlesh
Copy link
Member

benlesh commented Aug 27, 2015

So flatMap has a concurrency argument already... It seems more like we don't have flatMapLatest implemented... that surprises me a bit as I thought we had that checked off on #100. That one is a top priority for me.

@benlesh
Copy link
Member

benlesh commented Aug 27, 2015

so it should be:

function flatMapLatest<T, R>(project: (value:T) => Observable<R>, concurrent: number = Number.POSITIVE_INFINITY): Observable<R>

again this is a top priority for what I need, so if someone doesn't pick this up in the next few minutes I'm all over it.

@benlesh benlesh added the help wanted Issues we wouldn't mind assistance with. label Aug 27, 2015
@benlesh benlesh changed the title flatMapLatest with concurrency parameter feat(operator): add flatMapLatest with concurrency parameter Aug 27, 2015
@staltz
Copy link
Member Author

staltz commented Aug 27, 2015

Bikeshedding: why the concurrent: number as the last parameter?

@xgrommx
Copy link

xgrommx commented Aug 27, 2015

As I understood:

flatMap => flatMapWithMaxConcurrent(Number.POSITIVE_INFINITY)
flatMapConcat => flatMapWithMaxConcurrent(1)

More you can read in our discussion with @paulpdaniels Reactive-Extensions/RxJS#793 (comment)

@benlesh
Copy link
Member

benlesh commented Aug 27, 2015

So digging into this... what is the point of the concurrent parameter in flatMapLatest? How can you concurrently be subscribed to inner observables if you're only ever subscribed to the latest one.

I'm not sure how I missed that, I was more freaking out that it hadn't been implemented yet.

@staltz
Copy link
Member Author

staltz commented Aug 27, 2015

How can you concurrently be subscribed to inner observables if you're only ever subscribed to the latest one.

with concurrent = 2 you would be subscribed to the latest two inner observables.

@benlesh
Copy link
Member

benlesh commented Aug 27, 2015

with concurrent = 2 you would be subscribed to the latest two inner observables.

Oh... interesting. Yeah, I don't see why not. I guess it should default to one then ;) haha.

@benlesh benlesh self-assigned this Aug 27, 2015
@xgrommx
Copy link

xgrommx commented Aug 27, 2015

Hi @Blesh. Where can I take a look on all implemented methods(some list of methods)?

@benlesh
Copy link
Member

benlesh commented Aug 27, 2015

So, we have flatMapLatest, it's just been conflated into switchLatest for whatever reason. I didn't catch that before. This goes back to bike-shedding those names to make them more predictable. It doesn't have the concurrency bit though, I can probably add that.

@benlesh
Copy link
Member

benlesh commented Aug 27, 2015

@xgrommx you can find them all either in src/operators or in src/Rx.ts or src/Observable.ts

@staltz
Copy link
Member Author

staltz commented Aug 27, 2015

Oh yeah how could I forget that flatMapLatest was renamed to switchLatest. We talked so much about mergeMap/switchMap/concatMap. By the way, will switchLatest be named switchMap?

@benlesh benlesh removed the help wanted Issues we wouldn't mind assistance with. label Aug 27, 2015
@benlesh benlesh removed their assignment Aug 27, 2015
@benlesh
Copy link
Member

benlesh commented Aug 27, 2015

By the way, will switchLatest be named switchMap?

I think this requires a lot more discussion, honestly.

Digging into this issue, to do it properly is non-trivial. I've found a few other issues I'd like to refactor before I tackle this. I'll need to think about this a little more.

@trxcllnt
Copy link
Member

@Blesh @staltz since SwitchLatestSubscriber extends FlatMapSubscriber, and FlatMapSubscriber extends MergeSubscriber, we could extend the concurrent behavior to SwitchLatestSubscriber (it currently defaults to Number.MAX_VALUE).

At first glance, I think we'd only need to make the following four changes:

  1. track inner subscriptions in MergeSubscriber locally
  2. change _innerComplete to accept the inner Subscription, so we can remove it from the local subscriptions list (we're already planning to do this so we can remove it from the shared Subscription)
  3. change when SwitchLatestSubscriber switches to a new inner Observable
  4. parameterize how we dequeue the next inner Observable (SwitchLatestSubscriber would want to pop instead of shift)

@benlesh benlesh changed the title feat(operator): add flatMapLatest with concurrency parameter feat(operator): add concurrency parameter to switchLatest Aug 31, 2015
@benlesh
Copy link
Member

benlesh commented Aug 31, 2015

I've renamed this issue to make it more accurate as to the request.

@benlesh benlesh changed the title feat(operator): add concurrency parameter to switchLatest feat(operator): add concurrency parameter to switchMap Sep 18, 2015
@benlesh
Copy link
Member

benlesh commented Sep 18, 2015

Closing in favor of #347

@benlesh benlesh closed this as completed Sep 18, 2015
jayphelps added a commit to jayphelps/rxjs that referenced this issue Nov 23, 2016
jayphelps added a commit to jayphelps/rxjs that referenced this issue Nov 23, 2016
jayphelps added a commit to jayphelps/rxjs that referenced this issue Nov 23, 2016
jayphelps added a commit to jayphelps/rxjs that referenced this issue Nov 23, 2016
benlesh pushed a commit that referenced this issue Dec 5, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants