Rxjs: Trial change: squish `map` and `mapTo` back together.

[benlesh]

In an effort to reduce operator proliferation, I'd like to try squishing map and mapTo back into one operator (ala Rx4) to observe the difference in performance. A _long_ time ago it had an effect, but given the numerous changes since then, I'd like to investigate this.

[benlesh]

So digging into doing this I've explored a two options:

Option 1: have map handle non-function arguments like Rx4

Results:

• About a 2% drop in performance from master for both map and mapTo (now just map).
• slightly more complicated code in the map operator.
• one less operator (and puts mergeMapTo, switchMapTo etc in the crosshairs to be removed)

Option 2: Remove map(object) completely and force users to do map(() => object) like they'd have to anywhere else.

Results:

• map performance same as master for the more common use case of map(fn)
• cut perf in half when you needed to map to a specific object over and over.
• one less operator (and puts mergeMapTo, switchMapTo etc in the crosshairs to be removed)

[benlesh]

The case for removing mapTo-type functionality

I strongly believe that mapping to a static value over and over again is probably edge-casey and in a lot of cases an anti-pattern.

Reasons mapTo is an anti-pattern:

1. You're introducing external state into your operator chain. It's almost 100% likely that external state is mutable.
2. It's likely better to create a new (hopefully immutable) object on each "next" with map(() => ({ ...foo })) than it is to use mapTo(foo) in almost every case.

So I think we should remove it and other like it. I guess I'm leaning towards Option 2 above.

[staltz]

I strongly believe that mapping to a static value over and over again is probably edge-casey and in a lot of cases an anti-pattern.

Disagree very much. We see it very often in Cycle.js development (put aside that we now have xstream, people may build their apps in a similar way, and map/mapTo has nothing to do with xstream's focus on small size and hot-only). For instance just a few hours ago someone asked about something in their code, and among others, there it was:

const open$ = sources.DOM
    .select('.open').events('click').map(ev => 'open');

A common case is: I want to map click events to something, but I don't care about the click's metadata, just about its instantaneous moment.

mapTo is one of the good ideas of RxJS v5 that I decided to bring to xstream. It's not just the performance, aesthetically it looks better and reads better.

[benlesh]

mapTo is one of the good ideas of RxJS v5 that I decided to bring to xstream. It's not just the performance, aesthetically it looks better and reads better.

Interesting perspective, @staltz, thanks.

On the other hand you wrote this the other day which led me to believe this wasn't the case and that shrinking operator count was extremely important to the community, so much so it was worth forking it to some degree:

only about 20 of them are commonly used in Cycle.js apps, such as: map, filter, startWith, combineLatest, merge, of, take, scan, skip, flatMap, flatMapLatest, interval, let, distinctUntilChanged, withLatestFrom, takeUntil, concat, last, debounce.

The presence of all the other 155+ operators in RxJS creates noise when choosing operators.

[kwonoj]

I'm voting to remove mapTo. I agree its usefulness and I sometimes use it, but in most cases I don't see strong reason to have it as separate operator (or adding polymorphic behavior into map).

One of primary decision in RxJS 5 api surface was, if it could be achieved trivially using existing operator let it do it instead of adding new operator. I'd consider map(()=>obj) is case rationalize to remove mapTo(object).

[benlesh]

One flip side to this is that "RxJS 5 is modular now, so we don't have to worry about how many operators there are".

But then again, we have to maintain every "official" operator. And we proliferate the idea that "Rx is complex" because "I counted operators".

[staltz]

I actually agree on removing mapTo just to get the operator count decreased, but not on the basis of it being an anti-pattern.

[jadbox]

Is there a performance benefit to using mapTo to set the stream to a value vs passing a static returning function (x=>'val')?

[benlesh]

@jadbox there was, but I'd like to see if it still matters

[benlesh]

I played with this, and there was a minor hit to perf. In the end, I don't think it matters to anyone but those who aren't bundling. I'm leaving mapTo for now.

[masaeedu]

@blesh Couldn't mapTo be substituted with an additional overload in map? The actual invocation of the operators isn't the hot path in most people's code, so adding some extra code to distinguish between a lambda and a value as the first argument shouldn't cause any noticeable perf degradation. Rx .NET at least heavily relies on overloads to keep the core operator count small. I'd happily pay a 2% degradation in the fraction of a second it takes to setup my subscriptions for better development time experience.

[masaeedu]

Oh wait, didn't notice this still has a PRs welcome label. Does that mean you'd be open to the change?

[lock[bot]]

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.