-
Notifications
You must be signed in to change notification settings - Fork 5
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
Improvement: make it work for both listenable and iterable sources #2
Comments
Hi, Thanks for reporting this. I'll try to implement this soon then :) |
I've checked how other libraries handle this: console.log('start', Date.now())
Rx.Observable.of(1, 2, 3, 4, 5).debounceTime(50).subscribe({
next(val) {
console.log(Date.now(), 'next', val)
},
complete(val) {
console.log(Date.now(), 'complete')
},
}) xstream var debounce = require('xstream/extra/debounce').default
console.log('start', Date.now())
require('xstream').default.of(1, 2, 3, 4, 5)
.compose(debounce(50))
.addListener({
next(val) {
console.log(Date.now(), 'next', val)
},
complete() {
console.log(Date.now(), 'complete')
},
}) |
@Andarist I'm surprised because I'm almost sure that I tested that with RxJs :D and it behaved as I described, but after reading your comment I checked it and you're right: it emits the last value and completes immediately without debouncing. |
Hey @franciscotln @Andarist So regarding the |
From the definition we can read on callbag-basics github "Imagine a hybrid between an Observable and an (Async)Iterable, that's what callbags are all about." so iterables can also be async (generator) and I believe it's better to have all operators supporting both listenable/pullable sources. I can't think of an example right now but for me it just feels right to support both, behaving for example as Rx.Observable.from([1,2,3]).debounceTime(1000) // => 3| I just finished re-writing |
Personally I don't have a use cose for this, I was just playing with callbags & "fixed" this issue when looking at this repo. I believe though that asyncIterables might benefit from that (as mentioned by @franciscotln). I also try to support both types in my published operators, although it's not always possible (to my understanding).
No problem. This can wait, take care of yourself 👍 |
[EDIT] Ok while writting my reply, I realized this could be a valid scenario. I don't think it's a good idea to compare Rx's The thing is, with Rx it is still Reactive Programming (meaning the source decides when to emit). In this case, it's the consumer that decides when to pull a value. In this case we would have a pretty hard-to-understand workflow where the consumer wants a value but get it a few |
There was a lot of discussions about this in the most world, this issue for example. You may want to lean on it. |
Hi @Sinewyk Thanks for pointing this out. Interesting discussion indeed, but I don't really think it actually relates to the discussion here. The problem here is that we have pull streams (meaning the consumer can actually decide when it wants to get a value from a source). The question is, if a consumer requests a value, is it expected that the value is debounced or is it supposed to be sent right away? In my opinion, when the consumer requests the value it's actually its job to not request it at a reasonable rate. The responsibility shifts from the source to the consumer, but that's open for debate. |
I think this operator should behave like RxJs debounceTime. Right now it almost behaves like that for listenable sources, but it doesn't for iterables.
For example, this is the current behaviour for iterables:
It should ignore all previous values that were emitted within the delay range, so it should emit only the number 3 after 1 second.
We can achieve that but changing your code to something like:
What do you think?
The text was updated successfully, but these errors were encountered: