cujojs / most

Ultra-high performance reactive programming
MIT License
3.5k stars 231 forks source link

Dispose the source disposable #472

Closed ppoliani closed 6 years ago

ppoliani commented 6 years ago

When disposing a debounce stream we also need to dispose the source disposable

Summary

I was trying to use debounce in the following scenario

combine(...streams$)
    .debounce(1000)
    .chain(chainStream)
    .subscribe(observer);

But I soon realized that when I tried to dispose the above stream, it would ignore disposing the stream (source stream) that comes before the debounce.

Todo

briancavalier commented 6 years ago

It'd be great to add a regression test for this as well. There's a FakeDisposeSource in test/helper that is helpful in writing tests to verify disposal, and here's an example of using it (there are other examples in the test folder as well).

briancavalier commented 6 years ago

Looking good @ppoliani. Thanks for adding the test. I was porting your fix to @most/core in mostjs/core#107, and I noticed that there's actually another bug in the DebounceSink constructor. It's not necessary to use dispose.all (or disposeBoth in @most/core), and the source disposable can just be assigned directly to this.disposable.

Take a look at mostjs/core#107 and see what you think. If that seems right, then you can do the same here.

ppoliani commented 6 years ago

@briancavalier The change makes sense 😄

I'll include it here, as well.

briancavalier commented 6 years ago

Awesome, thank you, @ppoliani. We'll get this out in the next release soon.