Remove secondary mappings for flattening operators

[benlesh]

For operators like mergeMap, switchMap, concatMap, etc, we have a secondary selector. It's not used a lot in practice that I've seen. Originally, it added a lot of value in terms of perf because it prevented a closure... however, in practice it makes it harder to isolate errors from observation chains, and in a world with Turbofan, I'm not sure the performance benefit is there. We could eliminate a good amount of code in the library written to support this.

Use and Alternatives

Observable.of(1, 2, 3).mergeMap(a => range(0, a), (a, b) => a + b);

/// which is mostly the same as

Observable.of(1, 2, 3).mergeMap(a => range(0, a).map(b => a + b))

// however if there's an error in that second map, you're forced to kill the whole observable...

Observable.of(1, 2, 3)
  .mergeMap(a => range(0, a), (a, b) => { throw new Error('lol') })
  .catch(err => empty())

// rather than this, which is much more common

Observable.of(1, 2, 3)
  .mergeMap(
    a => range(0, a)
      .map(b => a + b)
      .catch(err => empty())
  )

Proposed Change

Eliminate the second argument to mergeMap, concatMap, switchMap, et. al. and instruct people to use .map within the projection function if they want a secondary level of mapping.

[wmaurer]

I've not a lot to technically add to this discussion.
I can only say that I very often use the secondary selector, and my team has followed my practice. This change would cause us to have to touch a lot of files across quite a number of projects.
However according to your observations, we're exceptions to the rule ...

[benlesh]

@wmaurer hi, Wayne... generally it's not a bad practice. Overall, it's a little better, performance-wise, for older browsers and runtimes because it eliminates some closure and a subscription. However, that perf difference probably doesn't matter a ton in practice. The biggest thing about it is in situations where it might error (which is admittedly uncommon), you can't isolate the error to the inner observable. So in the two examples below one will stop, and the other will not:

// With this one, the interval will die on the first error
Observable.interval(1000)
  .mergeMap(
    x => Observable.of(x),
    y => {
      if (y % 2 === 0) throw new Error(y)
      return y
    }
  )
  .catch(err => {
    console.error(err);
    return Observable.empty();
  })
  .subscribe(x => console.log(x));

/// With this one, the interval will live on. (usually desired)
Observable.interval(1000)
  .mergeMap(
    x => Observable.of(x)
      .map(y => {
        if (y % 2 === 0) throw new Error(y)
        return y
      })
      .catch(err => {
        console.error(err);
        return Observable.empty();
      })
  )
  .subscribe(x => console.log(x));

I mean, it's all down to use case. But the latter example above is (anecdotally) more common.

[benlesh]

More food for thought: Do we want to have symmetry with flatMap from the tc39 proposal for Array.prototype.flatMap ?

The signature there is: flatMap(mappingFn, thisArg) .... I hate thisArg... haha. But symmetry is a good thing.

[mattpodwysocki]

@benlesh FWIW, IxJS implements all according to spec for both flatten and flatMap.

• Iterable.prototype.flatMap
• Iterable.prototype.flatten
• AsyncIterable.prototype.flatMap
• AsyncIterable.prototype.flatten

[paulpdaniels]

I personally have never used the second selector for performance reasons, but I do tend to use (and suggest it) a lot. I personally find it ergonomically much nicer because it keeps my indentation level smaller. Also because the second selector doesn't deal with streams or iterables, I tend to treat it more like a pure selector that will never throw.

I won't raise a fuss if the argument is for better alignment with the spec or reducing api surface area, but I don't think it should be removed for "disuse".

[paulpdaniels]

Though I just realized with the "operator-as-a-library" paradigm if it was removed I could just publish mergeMapClassic.

[benlesh]

I don't think it should be removed for "disuse".

Yeah, I definitely don't mean that to be the primary argument. The primary arguments are around: 1. alignment with the spec, and 2. library size. (although it's not a lot, it would add up across a few operators)

[wmaurer]

Thanks for the explanation @benlesh. Like @paulpdaniels I've always treated the secondary selector as a pure function so I've never done anything "throwable" in there.
To be honest, I've nothing against this proposal, and am happy to start by initiating a change to our current practice now. That'll make the future transition easier.
Cheers!

[benlesh]

With regards to that argument I made above, the problem isn't so much with the "throwable" part as it is in patterns like you'd see in redux-observable or ngrx, where you might have something that throws, moving the catch outside of the flattening operation will kill your entire stream, but catching it inside gets weird with that secondary map:

// redux observable epic or ngrx effect
action$ => action$.ofType('LOAD_DATA')
  .switchMap(() => 
    makeAjaxCall()
      .catch(err => {
        console.log('ajax failed')
        return Observable.of({ type: 'AJAX_ERROR', err })
      }),
   (responseOrError) =>
    responseOrError.type === 'AJAX_ERROR'
      ? responseOrError
      : { type: 'AJAX_RESPONSE', response: responseOrError }
  )

As you can see it gets ugly fast... But by just using a map it cleans it up.

// redux observable epic or ngrx effect
action$ => action$.ofType('LOAD_DATA')
  .switchMap(() => 
    makeAjaxCall()
      .map(response => ({ type: 'AJAX_RESPONSE', response }))
      .catch(err => {
        console.log('ajax failed')
        return Observable.of({ type: 'AJAX_ERROR', err })
      })
  )

[benlesh]

... but honestly the biggest issues are library size and symmetry with the larger JavaScript ecosystem

[wmaurer]

@benlesh thank-you, that's a very nice argument for not using the secondary selector. Now I'm off to review my ngrx/effects code!

[masaeedu]

This is semi-relevant given that you discussed error handling above: I think the newly freed up second argument would be a good place for an "error selector", which allows you to flatmap errors in the source observable or in the projection to elements. Basically the same signature and semantics as catch, but as an optional second argument to flatMap.

This would alleviate some of the verbosity of doing catch inside the flatMap projection, and would have a rather nice symmetry with the "bind" operation of Promises, which also takes two arguments (one for projecting successful values, another for projecting failures).

[benlesh]

GIven that IxJS and the spec are just flatMap(fn, thisArg) I think we should follow suit. @kwonoj?

[martinsik]

I think one useful feature of selector functions is that they accepts parameters "unwrapped". For example:

Observable.combineLatest(other$, another$, (result1, result2) => result1)

or even easier with:

Observable.combineLatest(other$, another$, result => result)

With map() I'd have to use this:

Observable.combineLatest(other$, another$)
  .map(([result1, result2]) => result1)

or this:

Observable.combineLatest(other$, another$)
  .map(results => results[0])

[benlesh]

@martinsik I wasn't even thinking about combineLatest or zip, mostly thinking about the mergeMap, switchMap, concatMap and the like.

[benlesh]

This is now targeted at v8 to reduce breakage.