ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.64k stars 3k forks source link

Bike-Shedding: renaming `flatMap`, `flatMapLatest` for consistency and ease of adoption #167

Closed benlesh closed 9 years ago

benlesh commented 9 years ago

Proposed Change

This was brought up by @jhusain and I agree, flatMap isn't the easiest way to explain the operator.

The proposal is to change to a structure of [mergeStrategy][MappingStrategy] for names:

merging strategies are:

mapping strategies are:

So for example:

cc/ @trxcllnt

(edit: adding zip and combine) I suppose we can merge them with All defaulting to an Array.

benlesh commented 9 years ago

... I guess by this idiom, perhaps zipArray would be zipAll and zip would be zipMap? There could be a zipMapTo, also I suppose.

benlesh commented 9 years ago

Again, this is complete and total bike-shedding since this is a full semantic version different.

staltz commented 9 years ago

The proposal is to change to a structure of [mergeStrategy][MappingStrategy] for names

Big thumbs up for this proposal.

I think these are two different categories:

The first always takes one Observable as input, the second category always takes at least two Observables as input. I'd say the former are expansion-mapping operators, and the latter are combination operators. (word "expansion" because a value from the source Observable is mapped to an Observable of many values on the resulting Observable)

While I think zip name is perfect (really easy to explain with the zipper metaphor), I think we could do something about combineLatest and withLatestFrom.

I think Kefir has the best combinator operator. combine takes a set of active Observables and a set of passive Observables: Kefir.combine(obss: Observable<any>[], passiveObss?: Observable<any>[], combinator?: Function) Active Observables, when emitting next, are those that produce a next event on the resulting Observable, with the combination of the latest value from all (either active or passive) Observables. Passive Observables, when emitting next, do not incur a next event on the resulting Observable.

So the static combineLatest(a$, b$, combinator) is combine([a$, b$], [], combinator) and instance a$.withLatestFrom(b$, combinator) is combine([a$], [b$], combinator).

Hence combine is more general, so combineLatest and withLatestFrom are just special cases of it.

I like Elm's name for combineLatest, which is map2, map3, map4, etc depending on how many Signals are being combined. Perhaps one candidate for combineLatest, in this light, would be multiMap or combineMap, because the idea here is "map many Observables together, combined". So while I said up there that I didn't like combineMap, that's because it is not in the expansion-mapping category, so it should not be read as [mergeStrategy][mappingStrategy]. On the other hand, if it's called combineMap, it can and probably will be easily confused to be in the expansion-mapping category.

Regarding withLatestFrom, I have been teaching it as simplyMapThisObservableButPleaseAlsoUseTheLatestEventsFromTheseObservables. People totally get it like that, so maybe we need the name map in withLatestFrom. Also noteworthy: withLatestFrom is always an instance Observable, so the way a.withLatestFrom(b, fn) will be read is always "Observable A with the latest from Observable B". Candidates: mapUsingLatest ("A mapped using the latest B"), mapWithLatest ("A mapped with the latest B").

Another way of thinking about the combine flavors is: combineLatest is symmetric (all members are active), withLatestFrom is asymmetric (some are active, some are passive). So, tossing symmetricCombine and asymmetricCombine just as ideas, even though they are finger-twisters to type.

Yet another tossed idea is to have map be polymorphic:

I'm not entirely convinced of all these names, so I'm open to hear some comments, but here is my best bet so far:

Expansion-mapping operators:

Combination operators:

staltz commented 9 years ago

Ok, giving second thought to it, zipMap might be fine.

axefrog commented 9 years ago

If I am to throw in my $0.02, I would suggest that the mapping strategy need only be specified when it differs from the default "as-is" mode. So zip would emit values as-is, though combined of course, whereas zipMap is named as such because you're explicitly naming your intention to do something with the inputs other than the default.

Also I think it should be painted forest green.

trxcllnt commented 9 years ago

@staltz @blesh zip doesn't need to be renamed to zipMap because there's no map between source events and inner Observables. Since zipAll exists, it'd probably be logical to have zip, zipMap, and zipAll just to maintain parity with merge, mergeMap, and mergeAll, though I have my doubts about the utility of zipMap.

trxcllnt commented 9 years ago

@blesh and while zip, combineLatest, and withLatestFrom do merge observables together, I'd argue (and I think @benjchristensen would agree) they're really hybrid merge + buffer strategies. Their selector isn't for mapping between events and inner Observables, they're for reducing the buffered values to a single value. I vote for keeping the current zip, combineLatest, and withLatestFrom names, or maybe just rename withLatestFrom to joinLatest?

staltz commented 9 years ago

@trxcllnt

zip doesn't need to be renamed to zipMap because there's no map between source events and inner Observables.

Isn't there? Isn't the selector function a way of mapping from source events to resulting Observable?

I'd say new zip could behave like zipArray in RxJS v3, and new zipMap would be behave like zip in RxJS v3 with selector. Because of parity with merge, which doesn't take a selector function.

trxcllnt commented 9 years ago

@staltz nope, zip's selector function is for reducing buffered events to a single event, not for mapping input events to inner merged Observables.

staltz commented 9 years ago

So does Elm's map3 reduce three events to a single event, yet it's called map.

staltz commented 9 years ago

just rename withLatestFrom to joinLatest

Why? No rationale given behind this one, and it doesn't read as clearly as "A mapped with latest from B".

trxcllnt commented 9 years ago

@staltz: So does Elm's map3 reduce three events to a single event, yet it's called map.

yes, but Elm is a Haskell dialect and numbers in Haskell methods denote the argument count, a limitation we don't have in JS.

mergeMap gets its name from the fact that the operator merges after a map (coincidentally in Haskell, bind is defined in terms of fmap and join [a.k.a. Observable's merge]). concatMap concats after a map, and switchMap switches after a map.

The reason [the current] zip shouldn't be renamed to zipMap is because zip doesn't zip after a map... it maps after zipping.

I'm not saying zipMap wouldn't be a useful operator, just that if we're naming things with @blesh's proposed formula, [mergeStrategy]``[MappingStrategy], zip should stay named zip, and we introduce a new zipMap operator that maps then zips. For instance:

// mergeMap
Observable.prototype.mergeMap = (selector) => {
  return this.map(selector).mergeAll();
};
// zipMap
Observable.prototype.zipMap = (selector) => {
  return this.map(selector).zipAll();
}
// zip
Observable.prototype.zip = (...others, selector) => {
  return Observable.zip([this].concat(others), selector);
}

Another way to put this is that mergeMap's selector has the signature (x: T) => Observable<R>, whereas zip's selector has the signature (...x: T) => R, so they don't fit into the same classification of flattening strategies.

Why? No rationale given behind this one, and it doesn't read as clearly as "A mapped with latest from B".

Just thought I'd throw it in while we're bike-shedding about operators. withLatestFrom has always stuck out to me, as it doesn't fit the naming convention of any other operators. It is unique in its functionality (no other single operator does exactly what it does), but seemed like it would fit in a classification group like the others do. Last night I realized it's probably a new kind of join strategy, thus the name joinLatest.

staltz commented 9 years ago

The reason [the current] zip shouldn't be renamed to zipMap is because zip doesn't zip after a map... it maps after zipping.

Good point.

Last night I realized it's probably a new kind of join strategy, thus the name joinLatest.

Here's the RxJava issue where withLatestFrom came into existence, proposed by @benjchristensen. The discussion probably sheds some light on where did the name come from. I'd avoid join because there is already join in RxJava, and would confuse people to diverge in semantics. It's still a big advantage of Rx that operators are named roughly the same across languages/platforms. And that makes me realize we should try to be a bit conservative, or write a migration guide, or affect RxJava to do a similar major rename.

UPDATE: the discussion from that issue reminded me of a name I proposed for withLatestFrom: sampleMap or sampleCombine. However this doesn't fit [mergeStrategy] [mapStrategy] because the sample is done before mapping, not after.

trxcllnt commented 9 years ago

@staltz I'm aware of the join operator, and that's exactly why I'm suggesting this name: it seems like withLatestFrom is a specialization of join. There's join, groupJoin, and I'd like to propose a new one, joinLatest.

benlesh commented 9 years ago

While we're on join, I'll be honest: forkJoin never made sense to me, personally. joinLast or something? I dunno... (yay bike-shedding)

benlesh commented 9 years ago

Counter-proposal: Should the formula be [mapStrategy][MergeStrategy]? Since that's the order it happens in...

staltz commented 9 years ago

I could live with joinLatest (in contrast with combineLatest), but for the sake of beginners, it's not a friendly name. "combine" and "join" are synonyms in English, so for Rx training we have to assign them different semantics and it isn't obvious which one is which. I prefer names that already suggest the semantics.

staltz commented 9 years ago

Counter-proposal: Should the formula be [mapStrategy][MergeStrategy]?

That makes perfect sense. If we let go of familiarity, mapMerge is better than mergeMap.

staltz commented 9 years ago

And if we do go with [mapStrategy] [mergeStrategy], then it would make sense to use sampleMap ("withLatestFrom") and combineMap ("combineLatest").

Right now sampleMap is really resonating with me since it indicates order (first sample, then map), "sample" is really accurate as a merge strategy, and it sounds good.

trxcllnt commented 9 years ago

@staltz as @benjchristensen mentioned in the RxJava thread, withLatestFrom isn't sampling anything in the traditional sense. @jmhofer hit the nail on the head, it's a specialization of groupJoin, as you can see in this jsbin example.

benjchristensen commented 9 years ago

FYI that we tried adopting mergeMap instead of flatMap in RxJava and eventually gave up because the entire world uses flatMap. We stuck with concatMap and switchMap, but deleted mergeMap even though it is more symmetric.

benjchristensen commented 9 years ago

I vote for keeping the current zip, combineLatest, and withLatestFrom names

:+1: on keeping these.

mergeMap, switchMap, concatMap

:+1: on these names ... though I think that you should still have flatMap as an alias to mergeMap as it is ubiqitous.

mattpodwysocki commented 9 years ago

Agreed with @benjchristensen that flatMap should be the default for map/mergeAll behavior, because it is the defacto community standard for combining sequences.

I think we should have the following:

I don't like switchMap because we have switchFirst and switchLatest already in RxJS so we need to be explicit about which value we're selecting.

staltz commented 9 years ago

@trxcllnt

as benjchristensen mentioned in the RxJava thread, withLatestFrom isn't sampling anything in the traditional sense.

How not? a.withLatestFrom(b, fn) samples b when a emits, then uses values from both a and b to map to a result.

--1-----2--3-------4--------|
-----a---------b-------c----|
(          sample           )
-----1---------3-------4----|
-----a---------b-------c----|
--1-----2--3-------4--------|
(       withLatestFrom      )
-----a1--------b3------c4---|

it's a specialization of groupJoin, as you can see in this jsbin example.

Cool, I didn't notice that before.

trxcllnt commented 9 years ago

@staltz How not? a.withLatestFrom(b, fn) samples b when a emits, then uses values from both a and b to map to a result.

Ah, I get where you're coming from now. This may be a misclassification on my part, but I've understood operators to fall into specific classification groups by their behavior. merge, concat, and switch are flattening operators, i.e. operators that work on Observables of Observables, but with specific logic around how the inner Observables are flattened. Similarly, sample, throttle, zip, combineLatest, buffer, window, join, and groupJoin are buffering operators, or operators that buffer events internally and emit based on custom logic.

zip and combineLatest buffer events and emit them on coincidence, buffer and window buffer events in non-overlapping groups of events, sample and throttle buffer events and emit via scheduling, and join and groupJoin buffer events and emit them based on custom logic.

The reason I don't believe the sampling name fits with this operator is that it isn't buffering the event then using a scheduler to emit the values. It's closer to zip and combineLatest, since it's emitting events on coincidence, but there's not a shared vocabulary for coincidence aside from join, so the join name feels more correct to me.

staltz commented 9 years ago

sample and throttle buffer events and emit via scheduling.

Are you talking about the setTimeout scheduler? One use of sample is indeed source.sample(interval), but the other case is source.sample(otherObservable), and the latter is not emitting via scheduling. And that's the one I'm referring to. https://github.com/Reactive-Extensions/RxJS/blob/master/doc/api/core/operators/sample.md

In any case, if the word "sample" evokes traditional signal processing sampling by a constant frequency, another candidate word is "snapshot": snapshotMap.

trxcllnt commented 9 years ago

@staltz Yes, sample can "schedule" emission using another Observable, but it's not joining the sampled Observable and the "scheduler" Observable's values together. Join is.

staltz commented 9 years ago

Alright, you convinced me.

benlesh commented 9 years ago

I think all of these have been met, or are being met by other issues. Closing.