gcanti / fp-ts-rxjs

fp-ts bindings for RxJS
https://gcanti.github.io/fp-ts-rxjs/
MIT License
187 stars 29 forks source link

replace mergeMap with switchMap, fixes #27 #60

Open wmaurer opened 3 years ago

wmaurer commented 3 years ago

This solves the original problem reported in #27. As for the suggestion @OliverJAsh that we should be able to specify which merge strategy to use, sounds great, but can we implement this in a subsequent discussion and PR?

OliverJAsh commented 3 years ago

I'm not sure it's safe to assume that switchMap is a better "default" than mergeMap. Both operators have use cases, sometimes you want to run observables in parallel, sometimes you want to run one at a time.

wmaurer commented 3 years ago

This is not about running sequentially vs parallel. If it were so, we would be suggesting to replace mergeMap with concatMap. It's about switching to a new Observable on emission, cancelling the previous Observable. I would say this is a better default than mergeMap. I do agree, being able to specify which strategy is important, but can we at least get this through and continue the discussion about that in another issue?. Otherwise we're going to be stuck here for another year.

OliverJAsh commented 3 years ago

if we had to pick a default, mergeMap seems "safer" to me because it doesn't cancel. Someone might use chain without realising that it will cancel each time there's a new observable. It feels like it would be riskier to accidentally cancel than to accidentally not cancel.

(Ultimately I don't mind what we do, personally I'm not using chain, I just use the RxJS operators!)

wmaurer commented 3 years ago

Using the RxJS operators directly kind of defeats my purpose for using this library, i.e. the convenience it offers for ObservableEither, your new ObservableOption and my ObservableRemoteData data types.

But anyway, good argument about switchMap, but then I would argue that concatMap would be the correct default, as mergeMap could deliver the results of the inner observable out of order, which would also be rather surprising to the developer.

So, what would the API for various operators look like? one method for each operator? Something like chainWithConcatMap, chainWithSwitchMap, chainWithExhaustMap? Does that fit with the fp-ts API philosophy?

OliverJAsh commented 3 years ago

At Unsplash we've defined operators for each type using the same names as the built-in Observable operators, e.g.:

shared/facades/ObservableEither/operators.ts:

export const switchMap = <E, A, B>(
  fn: (a: A) => ObservableEither<E, B>,
): Rx.OperatorFunction<E.Either<E, A>, E.Either<E, B>> => (t$) =>
  pipe(t$, Rx.switchMap(E.fold(left, fn)));

shared/facades/ObservableRemoteData/index.ts:

export const switchMap = <E, A, B>(
  fn: (a: A) => ObservableRemoteData<E, B>,
): Rx.OperatorFunction<RemoteData.RemoteData<E, A>, RemoteData.RemoteData<E, B>> => (t$) =>
  pipe(
    t$,
    Rx.switchMap((remoteData) =>
      RemoteData.isSuccess(remoteData) ? fn(remoteData.value) : Rx.of(remoteData),
    ),
  );

We could prefix those names with chainWith if we feel that's necessary.

Lonli-Lokli commented 2 years ago

@OliverJAsh Have you tried defining support for Promises in your shared/facades/ObservableEither/operators.ts ?

I mean try to support mappers with export declare type ObservableInput<T> = SubscribableOrPromise<T> | ArrayLike<T> | Iterable<T>;