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

Expose concurrency parameter of mergeScan #868

Closed
kwonoj opened this issue Dec 3, 2015 · 10 comments
Closed

Expose concurrency parameter of mergeScan #868

kwonoj opened this issue Dec 3, 2015 · 10 comments

Comments

@kwonoj
Copy link
Member

kwonoj commented Dec 3, 2015

While MergeScanOperator has implementation to accept concurrency parameter

constructor(private project: (acc: R, x: T) => Observable<R>,
              private seed: R,
              private concurrent: number = Number.POSITIVE_INFINITY)

type definition of mergeScan does not allow to use it.

mergeScan<T, R>(project: (acc: R, x: T) => Observable<R>, seed: R).

Would it be better to expose concurrency parameter?

@benlesh
Copy link
Member

benlesh commented Dec 3, 2015

Probably, yes.

@kwonoj
Copy link
Member Author

kwonoj commented Dec 3, 2015

Cool, I'll try with expanding test coverage.

@kwonoj
Copy link
Member Author

kwonoj commented Dec 3, 2015

Maybe I'm incorrect, came to curious if concurrency of mergeScan is really being used compare to merge. @Blesh , may I possibly ask to shed some light to my understandings?

@benlesh
Copy link
Member

benlesh commented Dec 3, 2015

mergeScan is definitely looking at the concurrent value, it's just not being set from the actual operator function.

https://github.com/ReactiveX/RxJS/blob/f1dc76494cd0b134cd4682503cc479fc6afcbf8c/src/operator/extended/mergeScan.ts#L45

@kwonoj
Copy link
Member Author

kwonoj commented Dec 3, 2015

definitely looking at the concurrent value

: Yes, that's part where I started this issue. I'll think bit more to bring up some code snippets if possible.

@kwonoj
Copy link
Member Author

kwonoj commented Dec 3, 2015

It's simply my dumb understandings about questions above for usage of concurrency :( will continue work on this, thanks @Blesh .

@benlesh
Copy link
Member

benlesh commented Dec 3, 2015

I can't think of any use cases for changing the concurrency of this, really. But I can see the utility of it.

@trxcllnt, what's a use case for concurrency in mergeScan?

kwonoj added a commit to kwonoj/rxjs that referenced this issue Dec 3, 2015
- expose concurrency parameter to interface of mergeScan
- expand test coverage to test concurrency works

closes ReactiveX#868
kwonoj added a commit to kwonoj/rxjs that referenced this issue Dec 3, 2015
- expose concurrency parameter to interface of mergeScan
- expand test coverage to test concurrency works

closes ReactiveX#868
kwonoj added a commit to kwonoj/rxjs that referenced this issue Dec 3, 2015
- expose concurrency parameter to interface of mergeScan
- expand test coverage to test concurrency works

closes ReactiveX#868
@trxcllnt
Copy link
Member

trxcllnt commented Dec 4, 2015

@Blesh same as the case for merge. mergeScan(1) is actually concatScan().

@benlesh
Copy link
Member

benlesh commented Dec 4, 2015

@trxcllnt That's fair, I was just wondering more like "when would I use this"? That sort of use case.

@kwonoj kwonoj closed this as completed in fe0eb37 Dec 4, 2015
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@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

Successfully merging a pull request may close this issue.

3 participants