Skip to content
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

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

Closed
benlesh opened this issue Aug 14, 2015 · 29 comments

Comments

@benlesh
Copy link
Member

benlesh commented Aug 14, 2015

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:

  • merge
  • concat
  • latest (or switch)
  • zip
  • combine

mapping strategies are:

  • All - no mapping is done
  • Map - a function is called to do the mapping
  • MapTo - a value is provided to do the mapping

So for example:

  • flatMap(x => someObservable(x)) -> mergeMap(x => someObservable(x))
  • flatMap(Observable.just(32)) -> mergeMapTo(Observable.just(32))
  • flatMapLatest(x => someObservable(x)) -> latestMap(x => someObservable(x)) (or switchMap(x => someObservable(x)))
  • a.zip(b, (a,b) => a + b) -> a.zipMap(b, (a,b) => a + b)
  • Observable.zipArray(a, b, c) -> Observable.zipAll(a, b, c)
  • a.combineLatest(b, (a, b) => a + b) -> a.combineMap(b, (a, b) => a + b)
  • a.combineLatest(b, (a, b) => [a, b]) -> a.combineAll(b)
  • a.withLatestFrom(b, (a, b) => a + b) -> a.withMap(a, (a, b) => a + b)

Other questions

  • What about zip, combineLatest and withLatestFrom? which are all merge strategies... how can we fit them into this idiom? Are they different enough they don't require change?

cc/ @trxcllnt

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

@benlesh
Copy link
Member Author

benlesh commented Aug 14, 2015

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

@benlesh
Copy link
Member Author

benlesh commented Aug 14, 2015

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

@staltz
Copy link
Member

staltz commented Aug 14, 2015

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

Big thumbs up for this proposal.

  • mergeMap 👍
  • mergeMapTo 👍 although I'd use the example mergeMapTo(Observable.from([10,20,30])) because the just example seems redundant (doesn't show mergeMapTo's usefulness) since we can write just .mapTo(32)
  • switchMap 👍 I prefer this one over latestMap because latestMap might get confused with combineLatest, these are two very different things.
  • zipMap 👎 I think the name zip is perfect
  • combineMap 👎 but soon I'll say it's 👍, I'll explain.
  • combineAll 👎
  • withMap 👎

I think these are two different categories:

  • mergeMap, concatMap, switchMap
  • zip, combineLatest, withLatestFrom

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:

  • map(selectorFn) plain ol' map
  • map(observables: Observable<any>[], selectorFn) is combineLatest, name inspired by Elm's map2, map3, etc, dropping the numbers.

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:

  • mergeMap (instance)
  • concatMap (instance)
  • switchMap (instance)

Combination operators:

  • combine (static)
  • zip (static, instance)
  • combineMap (instance)
  • mapWithLatest (instance)

@staltz
Copy link
Member

staltz commented Aug 14, 2015

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

@axefrog
Copy link

axefrog commented Aug 14, 2015

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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member

staltz commented Aug 14, 2015

@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
Copy link
Member

@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
Copy link
Member

staltz commented Aug 14, 2015

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

@staltz
Copy link
Member

staltz commented Aug 14, 2015

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
Copy link
Member

@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
Copy link
Member

staltz commented Aug 14, 2015

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
Copy link
Member

@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
Copy link
Member Author

benlesh commented Aug 14, 2015

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
Copy link
Member Author

benlesh commented Aug 14, 2015

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

  • mapMerge
  • mapConcat
  • mapZip

@staltz
Copy link
Member

staltz commented Aug 14, 2015

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
Copy link
Member

staltz commented Aug 14, 2015

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

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

@staltz
Copy link
Member

staltz commented Aug 14, 2015

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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

👍 on keeping these.

mergeMap, switchMap, concatMap

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

@mattpodwysocki
Copy link
Collaborator

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:

  • flatMap
  • concatMap for map/concatAll
  • flatMapWithMaxConcurrency for map/merge(n)
  • flatMapFirst - previously from @jhusain and exclusive
  • flatMapLatest - still uses the RxJS standard

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
Copy link
Member

staltz commented Aug 15, 2015

@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
Copy link
Member

@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
Copy link
Member

staltz commented Aug 17, 2015

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
Copy link
Member

@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
Copy link
Member

staltz commented Aug 18, 2015

Alright, you convinced me.

@benlesh
Copy link
Member Author

benlesh commented Sep 18, 2015

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

@benlesh benlesh closed this as completed Sep 18, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants