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

Add bufferWithTimeout() and bufferWithInterval(). #227

Open
hastebrot opened this issue Nov 15, 2016 · 7 comments
Open

Add bufferWithTimeout() and bufferWithInterval(). #227

hastebrot opened this issue Nov 15, 2016 · 7 comments

Comments

@hastebrot
Copy link

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

rpominov commented Nov 15, 2016

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

hastebrot commented Nov 15, 2016

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

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

hastebrot commented Nov 15, 2016

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

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

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

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

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

No branches or pull requests

2 participants