cujojs / most

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

map + switch does not close previous streams (when using fromEvent) #488

Closed guojenman closed 6 years ago

guojenman commented 6 years ago

Summary

map + switch doesn't close the previous streams after switching to the latest (when using fromEvent)

Expected result

when we switch to a new stream, the old stream should be closed

Actual Result

values from old streams are ignored BUT the old streams themselves continue to run

Versions

Chrome

Steps to reproduce

In the below plnkr, click on the page a few times really fast. You should see something like this start 1 start 2 start 3 end 3 set 3 basically, "end 1" and "end 2" should not show because the streams should be closed when switched to the new one. But what you end up seeing is this start 1 start 2 start 3 end 1 end 2 end 3 set 3 As you can see, only "set 3" is shown, which is correct... but "end 1" and "end 2" should not be there, because those streams should've been close before ending.

Code to reproduce

This does not work - using fromEvent http://plnkr.co/edit/aH4uAMsWMt5nBXYQA5Rx?p=preview

Oddly enough, using most.from works http://plnkr.co/edit/ROK78a?p=preview

At the very least, they should behave exactly the same.

This is the fromEvent using RxJS, which works as expected http://plnkr.co/edit/R4lONpIQ1PhpTG9sFvTi?p=preview

briancavalier commented 6 years ago

Hi @guojenman, thanks for opening this issue, and for the test cases. I should have time to investigate later today.

briancavalier commented 6 years ago

Reproduce locally using the example test case.

briancavalier commented 6 years ago

@guojenman There's a candidate fix in #490. If you have a chance to pull and build and then try it out, I'd really appreciate feedback. All it needs is a unit test to prevent a regression, then we'll get it merged and released.

guojenman commented 6 years ago

@briancavalier I can confirm that this fix works. Thank you!

briancavalier commented 6 years ago

@guojenman Awesome, thanks for trying it out and letting us know.