kefirjs / kefir

A Reactive Programming library for JavaScript
https://kefirjs.github.io/kefir/
MIT License
1.87k stars 97 forks source link

Add bufferWithTimeout() and bufferWithInterval(). #227

Open hastebrot opened 7 years ago

hastebrot commented 7 years ago

Highland uses setTimeout() and Kefir uses setInterval() for the batch/buffer with time (illustrated below). It would be nice to have both functions in Kefir, with a distinctive name.

Using highlandStream.batchWithTimeOrCount(100 /*ms/*, Infinity):

[ 0, 0, 0 ]
[ 150, 200, 250 ]
[ 300, 350, 400 ]

Using kefirStream.bufferWithTimeOrCount(100 /*ms*/, Infinity):

[ 0, 0, 0 ]
[ 150 ]
[ 200, 250 ]
[ 300, 350 ]
[ 400 ]
rpominov commented 7 years ago

If you want to use it with count = Infinity then this should work in Kefir I think: stream. bufferBy(Kefir.interval(100)).

Update: Ah sorry, misunderstood which library uses setTimeout and which setInterval. This won't help then. Need to think more about it.

hastebrot commented 7 years ago

I thought about something like stream.bufferBy(Kefir.timeout(100, stream)), with a new Kefir.timeout() function. But I'm not sure if it is a good idea to subscribe to this single stream twice.

rpominov commented 7 years ago

Yeah, if we subscribe to the same stream twice here is how it can be done:

var stream = Kefir.stream(em => {
  setTimeout(() => {em.value(0); em.value(0); em.value(0)}, 0)
  setTimeout(() => {em.value(150)}, 150)
  setTimeout(() => {em.value(200)}, 200)
  setTimeout(() => {em.value(250)}, 250)
  setTimeout(() => {em.value(300)}, 300)
  setTimeout(() => {em.value(350)}, 350)
  setTimeout(() => {em.value(400); em.end()}, 400)
})

stream.bufferBy(stream.flatMapFirst(() => Kefir.later(100))).log()
hastebrot commented 7 years ago

Thanks, this works great. Does it make sense to add the functions outlined above to emphasize the difference between interval and timeout?

bufferWithCount(), bufferWithInterval(), bufferWithTimeout(). Maybe there is a good way to combine bufferWithCount() and bufferWithInterval() to get an equivalent to bufferWithTimeOrCount().

BTW: In TypeScript (and I think Flow) it is Kefir.later(100, null). I'll update the kefir.js.flow and bring the TypeScript declaration file in sync with it, when I have time.

rpominov commented 7 years ago

Not sure yet if we should add the methods. We will have too many buffering methods with slightly different behaviour. To my taste we already have a bit too many. Maybe we should keep this open and wait for more demand.

Maybe there is a good way to combine bufferWithCount() and bufferWithInterval() to get an equivalent to bufferWithTimeOrCount().

We don't have bufferWithInterval because the idea was that people should use bufferBy(Kefir.interval(n)) instead. But we haven't found a way to combine bufferWithCount and bufferBy(Kefir.interval(n)) so bufferWithTimeOrCount was added.

If Flow/TypeScript don't allow to omit second argument maybe it's for the best actually. Kefir doesn't have a special support for it neither, it just works as if undefined was passed. So maybe to require explicit argument is good in stricter environments like Flow/TypeScript.

hastebrot commented 7 years ago

Maybe we should keep this open and wait for more demand.

+1

If Flow/TypeScript don't allow to omit second argument maybe it's for the best actually.

At least TypeScript allows this and it is quite common.

-export declare function interval<T>(interval: number, value: T): Stream<T, void>
+export declare function interval<T>(interval: number, value?: T): Stream<T, void>
rpominov commented 7 years ago

Yea, I just not 100% sure about Flow/TypeScript, but I'll merge the PR if you create one.