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

replace mergeMap with switchMap, fixes #27 #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wmaurer
Copy link

@wmaurer wmaurer commented Apr 21, 2021

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

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

wmaurer commented Apr 21, 2021

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

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

wmaurer commented Apr 21, 2021

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

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

@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>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants