Conflict between mergeStatic and mergeMap

[david-driscoll]

RxJS version:
latest ( maybe beta2, not 100% sure).

Code to reproduce:

// merge concurrency
Observable.merge(o1, o2, o3, 4);

// vs

// mergeMap concurrency
o.mergeMap(p => Observable.fromPromise(p), 1);
o.mergeMap(p => Observable.fromPromise(p), (o, i) => i, 1);

Expected behavior:
Not sure what the expected behavior is (see additional)

Actual behavior:
mergeMap only accepts concurrency as the third parameter, though the signatures say otherwise.

Additional information:
Basically I noticed this issue yesterday, where I was trying to add concurrency to some mergeMap calls, and I noted that the signatures allow for concurrency as the second argument, if you have no result selector. When I ran the code (and looked at the code) this was incorrect. The code only expects concurrency as the last parameter.

This is almost certainly my doing (unintentionally I swear!). 😄

My question is, which is the way RxJS5 should land?

• Update mergeMap signatures to correctly reflect the location of concurrency as strictly the third argument.
• Update mergeMap operator to allow for concurrency as the second argument, if a result selector is not needed.

[kwonoj]

I would prefer explcit signature, such as mergemap(x, null, concurrency).

[trxcllnt]

@kwonoj @david-driscoll heh, I prefer the overloaded signature.

[benlesh]

I'm for the overloaded signature as long as it doesn't affect performance.

[david-driscoll]

This would be a minor change, just check to see if typeof(resultSelector) === "number" and then set concurrency to resultSelector and resultSelector to null. I guess there would be a minor creation time perf hit for that check (but not once we get into the Operator itself).

https://github.com/ReactiveX/RxJS/blob/master/src/operator/mergeMap.ts#L25

[kwonoj]

I'm ok with overloaded, explicit signature's my personal preference :)

Since we already have micro perf for mergeMap, we can run couple of tests to see if there's any perf regression. I'd assume there won't be critical issues.

[kwonoj]

Closed via #1453.

[lock]

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.