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

Document special cases of reduce operator usage for certain pattern #2323

Closed
NNemec opened this issue Feb 3, 2017 · 9 comments
Closed

Document special cases of reduce operator usage for certain pattern #2323

NNemec opened this issue Feb 3, 2017 · 9 comments
Labels
docs Issues and PRs related to documentation help wanted Issues we wouldn't mind assistance with.

Comments

@NNemec
Copy link

NNemec commented Feb 3, 2017

According to http://reactivex.io/documentation/operators/reduce.html, ReactiveX defines a reduce operator for nearly all languages, a 'collect' operator for only a few of them, not including Javascript.

'collect' seems to a a very valuable to RxJS. Using 'reduce' to collect items into an Array is rather awkward. Specifically, I cannot find a way to make sure that the empty array required as initial value is not reused for multiple subscriptions of the observable.

Please implement the 'collect' operator for RxJS in such a way that an new empty array is created as initial value for each subscription.

@paulpdaniels
Copy link
Contributor

paulpdaniels commented Feb 3, 2017

Would the rest operator not be sufficient in this case?

source.reduce((array, d) => [...array, doSomething(d)], [])

@jayphelps
Copy link
Member

jayphelps commented Feb 4, 2017

Besides paul's great example, in this particular case it sounds like you could use the .toArray() operator, which will indeed create a new array for each subscription:

const items = Observable.of(1, 2, 3).toArray();

items.subscribe(arr1 => {
  items.subscribe(arr2 => {
    console.log(arr1 !== arr2);
    // true (they are not same array instance)
  });
});

But one could argue for other mutable data structures than arrays. In that case, I'm not aware of any super clean way to do this with the existing operators. (others may think of a pattern)

There's defer(), which is generally handy for this sort of thing. Basically just wrap any part of your observable chain in a defer, so that it's unique per subscription:

const source = Observable.of(10, 20, 30); // from wherever
const items = Observable.defer(() =>
    source
      .reduce((arr, item) => {
        arr.push(item);
        return arr;
      }, [])
  )
  // do other stuff like
  .filter(d => d.length > 0);

It's not as trivial as collect might be though. Seems unfortunate to add another operator that's so dang similar. An overload for reduce, accepting an initialValue factory, but that would mean you couldn't pass a function as a seed--I don't know why the heck someone would need to, but could be dangerous maybe.

I'll let some others chime in here.

@jayphelps
Copy link
Member

jayphelps commented Feb 4, 2017

ooooo I just thought of a new solution:

const items = Observable.of(10, 20, 30)
  .reduce((arr = [], item) => {
    arr.push(item);
    return arr;
  }, undefined);

Notice that we're passing in literally the value of undefined as the initial value/accumulator. This is not the same as not passing any value at all. Reduce in RxJS (as well as the native Array.prototype.reduce) have a different behavior when you don't pass any value at all (not even undefined). But we can take advantage of the fact that default params are used if the argument is undefined.

@NNemec
Copy link
Author

NNemec commented Feb 4, 2017

Thanks, Jay - seems you exactly understood my issue and put it in better words than I would have been able to!
The last solution does indeed look significantly better than the workaround that I had in place.

The toArray solution does not really work for me, because in my case, each iteration may reduce zero to multiple values.

Paul's solution does indeed offer an elegant approach, but I somehow doubt th at I can trust a Javascript engine to optimize out the unnecessary copies to avoid the quadratic scaling.

I leave it up to you guys to close this issue if you decide that the solution proposed by Jay is good enough. I could imagine that the use-case is common enough to at least document this clever solution along with the reduce function.

NNemec added a commit to NNemec/korespo that referenced this issue Feb 5, 2017
@jayphelps
Copy link
Member

jayphelps commented Apr 7, 2017

Edit: he deleted his comment, here's a copy for context:

Is there a reason to do it that way specifically versus?

const items = Observable.of(10, 20, 30)
  .reduce((arr, item) => {
    arr.push(item);
    return arr;
  }, []);

@masaeedu yeah, see my comment below the example

Notice that we're passing in literally the value of undefined as the initial value/accumulator. This is not the same as not passing any value at all. Reduce in RxJS (as well as the native Array.prototype.reduce) have a different behavior when you don't pass any value at all (not even undefined). But we can take advantage of the fact that default params are used if the argument is undefined.

We want a brand new instance for every subscription, instead of sharing the same instance.

@masaeedu
Copy link
Contributor

masaeedu commented Apr 7, 2017

@jayphelps Yeah, sorry. I hadn't read the original post carefully enough and hadn't understood the problem. I get it now.

It seems like the only sensible way to bake this in to reduce would be to make it a breaking change where the factory overload is the only one available. You can still use a function as a seed value by returning a function from the factory. It'd be a pretty huge breaking change though, which is why making it as a separate collect operator would be a good idea.

@jayphelps
Copy link
Member

jayphelps commented Apr 7, 2017

If I had to make an educated guess, the core library itself prolly won't get collect, it's too simple for users to implement themselves with existing stuff and they've been trying to keep things slimmer cause too many operators is one of the biggest complaint for learning and file size.

If you don't like the undefined trick, it's easy enough to abstract it away:

Observable.prototype.collect = function (seedFactory, accumulator) {
  return this.reduce(
    (acc = seedFactory(), ...rest) => accumulator(acc, ...rest),
    undefined
  );
};

Observable.of(1, 2, 3)
  .collect(() => [], (arr, item) => {
    arr.push(item);
    return arr;
  });

If microperf is critical, it's easy enough to create a more performant version--chances are extremely high that your app won't know the difference.


Certainly an argument could be made that documenting these patterns would be super helpful for people who may not realize them. Anyone can jump in and help.

@kwonoj kwonoj added docs Issues and PRs related to documentation help wanted Issues we wouldn't mind assistance with. and removed type: discussion labels Apr 16, 2017
@kwonoj kwonoj changed the title collect operator Document special cases of reduce operator usage for certain pattern Apr 16, 2017
@kwonoj
Copy link
Member

kwonoj commented Apr 16, 2017

I believe we probably won't going to have this as separate operator set, but feeling worth to mention as some kind of examples - changed issue topic.

@benlesh
Copy link
Member

benlesh commented Aug 18, 2020

Closing this. I think that reduce is now adequately documented, and we don't need another operator here.

@benlesh benlesh closed this as completed Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issues and PRs related to documentation help wanted Issues we wouldn't mind assistance with.
Projects
None yet
Development

No branches or pull requests

6 participants