-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(mergeMap): allow conccurent to be set as the second argument for … #1453
fix(mergeMap): allow conccurent to be set as the second argument for … #1453
Conversation
what about add type definition test in this PR as well? |
820d79c
to
d8b90eb
Compare
Hadn't thought about it, added! |
d8b90eb
to
27740bb
Compare
👍 |
Just tried new 'reaction' feature to your PR :) |
I'll check this in around today to tomorrow if there isn't additional suggestion. |
@david-driscoll @kwonoj wait, I think this should be thought through a bit since we want to have the following: mergeMap(source, selector, resultSelector?)
mergeMap(source, selector) Perhaps we should break it out into its own thing such as: mergeMapConcurrent(source, maxConcurrent, selector, resultSelector?) |
@mattpodwysocki what are you saying is the |
I'll put on hold check in to clarify. Just in case, current mergeMap signature supports these. mergeMap(project);
mergeMap(project, concurrent);
mergeMap(project, resultSelector);
mergeMap(project, resultSelector, concurrent); |
The commit message has a typo. conccurent => concurrent |
…mergeMap and mergeMapTo This behavior is similar to how merge works
27740bb
to
bc0b0b3
Compare
@Blesh fixed 😄 |
expectSubscriptions(inner.subscriptions).toBe(innersubs); | ||
expectSubscriptions(e1.subscriptions).toBe(e1subs); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test for mergeMap(selecter, resultSelector, 2)
so make sure it covers that call type.
Also, go through the tests and make sure every permutation of mergeMap
has been called at least once. fn
, fn, fn
, fn, number
, and fn, fn, number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result selector version is handled already above this test. As well as the single method test is handled above as well. Two methods, without concurrency isn't in there just yet, but that can be added fairly easily.
LGTM. |
Merged with c003468, thanks @david-driscoll ! |
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. |
Description:
Allow
concurrent
to be supplied as the second argument formergeMap
andmergeMapTo
.Related issue (if exists):
see #1448 for discussion
Perf Details
On my machine the numbers were kind of hit or miss, so it's worth a second look by someone else as well. But the numbers look to not really be affected all that much.