Replace mergemap with switchmap #30
Conversation
Observable does not always satisfy Alternative's distributivity law
BREAKING CHANGE: chain now unsubscribes from passed observable on source emit
| of, | ||
| ap: (fab, fa) => combineLatest([fab, fa]).pipe(rxMap(([f, a]) => f(a))), | ||
| chain: (fa, f) => fa.pipe(mergeMap(f)), | ||
| chain: (fa, f) => fa.pipe(switchMap(f)), |
There was a problem hiding this comment.
Theoretical we could replace switchMap with exhaustMap and would also get the memory problem solved and hold all laws right?
In practice you are totally right that switchMap should be the default chain. Should we add for completeness alternative Monad instances like mergeMonad and exhaustMonad?
There was a problem hiding this comment.
AFAIK exhaustMap waits for passed Observable to complete before subscribing to the new created one (result of chain). Looks like an incorrect behavior and I can't think of a use case for that.
Replacing switchMap with mergeMap is a matter of const m = {...observable, chain: (fa, f) => fa.pipe(mergeMap(f))} - is it really necessary to put it into the lib? What about other possible operator replacements? IMO switchMap covers 99% usecases.
There was a problem hiding this comment.
AFAIK exhaustMap waits for passed Observable to complete before subscribing to the new created one (result of chain). Looks like an incorrect behavior and I can't think of a use case for that.
Just a theorical question sure for the use cases we are looking at it does different things.
Replacing switchMap with mergeMap is a matter of const m = {...observable, chain: (fa, f) => fa.pipe(mergeMap(f))} - is it really necessary to put it into the lib? What about other possible operator replacements? IMO switchMap covers 99% usecases.
For the ReaderObservable this approach would not be practical and it would follow the implementation style of fp-ts like done for taskSeq. But of cause it could be part of another PR. Totally agree with the fact that switchMap covers most of the usecases.
There was a problem hiding this comment.
follow the implementation style of fp-ts like done for taskSeq
Do you mean that there should be additional mergeReaderObservable and the same for any other instance using Observable as base?
There was a problem hiding this comment.
Thats what I would like to discuss to keep up with the fp-ts coding style. @gcanti any opinions?
There was a problem hiding this comment.
That's tricky :) There's no way to test .toNotBe on TestScheduler's expectObservable, so we need to pass assert.notDeepStrictEqual as equality assertion function. Then we intentionally test that resulting observable does not equal marbles, because they are "incorrect" but should be the same as in success' RIGHT SIDE.
There was a problem hiding this comment.
Waaaaaaaait... Let me recheck o_O
There was a problem hiding this comment.
Yeah, you are right, there's an error in the test. How could I miss it?... Anyway I updated the test and it still shows distributivity does not hold
There was a problem hiding this comment.
I have a proposal: for what I can undestand switching to switchMap is important to you, while a discussion about Alt / Alternative instances, albeit being very interesting from a theoretical point of view, is way less important in practice (it's just merge in the end), and currently is a blocker.
So what about:
- replace
mergeMapwithswitchMap(breaking change) - optimize
filterMapimplementation - remove
Alt/Alternativeinstances fromObservable,ObservableEitherandReaderObservable(it's a breaking change but we are already doing a breaking change forswitchMap) - release v0.7
- (maybe) continue the debate about Alternative in the future with less rush
@raveclassic @mlegenhausen what do you think?
There was a problem hiding this comment.
@gcanti We can keep zero as part of Plus typeclass, it doesn't require alt to be distributive. So that Observable is a Plus & Alt, but not an Alternative. This is not a breaking change, everything stays structurally equal.
But I'm open to any decision.
gcanti
left a comment
gcanti
left a comment
There was a problem hiding this comment.
@raveclassic I repeat my proposal, personally I don't like Plus precisely because is structurally equivalent to Alternative (no type checker help), that's why I removed Plus in v2
|
Ok, got it, I'll update the PR. |
|
How did we go on this one? Otherwise I may have to maintain a patched version of this repo if I am to use chain in projects. If there is anything pending for gcanti's proposal of changes, what are they? I'd be happy to lend a hand. |
|
I know bumping github issues and PRs is bad form, but I also think If nobody's willing to move this PR forward, should we create another simpler PR for the use of |
|
OK, let's see what happens, new PR here: |
5 participants
This PR replaces mergeMap with switchMap to avoid memory leaks.
contains parts of #29
closes #27