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

Remove mergeScan or provide better documentation for it #2737

Closed
1 of 2 tasks
SomeKittens opened this issue Jul 10, 2017 · 16 comments
Closed
1 of 2 tasks

Remove mergeScan or provide better documentation for it #2737

SomeKittens opened this issue Jul 10, 2017 · 16 comments

Comments

@SomeKittens
Copy link
Contributor

SomeKittens commented Jul 10, 2017

I have a problem with mergeScan, namely, I can't think of a single situation in which it'd be needed. The categories I see of problems almost needing mergeScan are:

  • Internal state is needed, but an inner observable is unnecessary (just use scan). The example in the docs is in this category
  • There is some operation (probably async) that requires an inner observable, but no internal state is needed (just use mergeMap). For instance, successive async calls.
  • There is an operation that requires both an async inner observable and internal state BUT is vulnerable to race conditions. In this case, mergeScan falls flat on its face, as the accumulator passed in could be stale by the time the inner observable returns. In this case, it's better to just For example, see this (entirely contrived) example where I sum the weight of the first 10 Pokémon: http://jsbin.com/qosiwenaqi/edit?js,console

In short, I don't see a use for mergeScan that isn't better served by another operator and worse, it can cause unintended race conditions.

So, as a "What do we do about this issue", I'd like either:

  • Remove the mergeScan operator from RxJS v5

or

  • Clarify intended uses of mergeScan in the docs so silly folks don't open elaborate issues demanding its removal

(Both @robwormald and @rgbkrk have discussed this with me in various Slacks so I'm cc'ing them here)

@kwonoj
Copy link
Member

kwonoj commented Jul 10, 2017

/cc'ing @trxcllnt too for visibility.

@benlesh
Copy link
Member

benlesh commented Jul 10, 2017

cc @trxcllnt ... this was his brain child back in the infancy of this lib.

@jayphelps
Copy link
Member

jayphelps commented Jul 10, 2017

Just for reference, here's the decision tree for it:

I have one existing Observable, and I want to compute a formula using all values emitted and output the computed values as a nested Observable when the source emits a value

- label: and output the computed values as a nested Observable when the source emits a value

@SomeKittens
Copy link
Contributor Author

output the computed values as a nested Observable

It's my understanding that the merge part of mergeScan unwraps the inner observable, only passing a value down the line?

@trxcllnt
Copy link
Member

mergeScan works like flatMap with an accumulated value. It isn't as commonly used as either flatMap or scan, but it isn't something that's easily accomplished by a combination of other operators, which is why it exists. There should also be a corresponding expandScan operator, but for various reasons I didn't get around to merging it in.

@SomeKittens
Copy link
Contributor Author

mergeScan works like flatMap with an accumulated value.

My query isn't about how mergeScan works - I'm up to date on that. I want to know when and why I would use mergeScan. After several discussions, we couldn't come up with a single, practical example where one would use this operator.

It isn't as commonly used as either flatMap or scan

In fact, the first ten pages of GitHub results searching for mergeScan in both JS and TS show no actual uses of the operator (only people who've checked Rx into their repo).

various reasons I didn't get around to merging it in.

Pun intended? 😂

@benlesh
Copy link
Member

benlesh commented Jul 25, 2017

We can consider removing mergeScan for v6... It's a breaking change. However, by that version we should have treeshaking working, so I don't see the benefit to removing it other than to keep us from needing to maintain it... which has been zero maintenance so far.

@ghetolay
Copy link
Contributor

Just so you know I'm using mergeScan to do progressive image display. I download and decode the image by chunk so I need the last chunk to compute the next and all this is done async using web worker.

But in fact I'm using concatScan because I set concurrent parameter to 1 that's why I'm not facing the race condition problem enounced.

@fjozsef
Copy link

fjozsef commented Aug 8, 2017

On our project we are also using mergeScan. The simplified scenario is the following: There is a state, which is reflected on the UI, which can be modified by the user, but not directly, just with actions. There is a state-action pair which is sent to the server and it computes the next state and sends back.

We've just run into the race condition problem and it would be great if there would be an exhaustScan, but if my results are correct then this kind of operator doesn't exist.

@paulpdaniels
Copy link
Contributor

There are certain paging scenarios where it makes sense as well, see: https://stackoverflow.com/questions/45383857/building-an-infinite-scrolling-list-in-typescript-with-rxjs5/45405933#45405933

Granted I still haven't found a use case for concurrency greater than 1, unless your data was order agnostic.

@wrotki
Copy link

wrotki commented Nov 23, 2017

mergeScan solves an important scenario for me, where I have a sequence of async calls. Say, first call gets an user record from a DB, second call is to external service and passes user's attributes obtained from the first call, third call is to yet another service, and requires some data from first and the second calls. Passing an accumulator with intermediate results which mergeScan does makes this pretty straightforward.

@benlesh
Copy link
Member

benlesh commented Nov 27, 2017

I think we need better documentation sure... but we're not going to remove it for now. :)

@ladyleet, can you please have the docs folks take a look at documenting this one better?

@benlesh benlesh closed this as completed Nov 27, 2017
@ladyleet
Copy link
Member

for sure @benlesh!

@ladyleet
Copy link
Member

Referenced here ReactiveX/rxjs-docs#169

@gilesbradshaw
Copy link

Please don't get rid of it very useful as a concatScan with concurrency = 1

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 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