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

Should .take() unsubscribe from the source when it's done? #35

Closed
myndzi opened this issue Jan 11, 2015 · 4 comments
Closed

Should .take() unsubscribe from the source when it's done? #35

myndzi opened this issue Jan 11, 2015 · 4 comments

Comments

@myndzi
Copy link

myndzi commented Jan 11, 2015

Test code:

'use strict';

var Kefir = require('kefir');

var thing = Kefir.fromBinder(function (emitter) {
    console.log("subscribed");
    emitter.emit(1);

    var timer = setTimeout(function () {
        emitter.emit(2);
    }, 1000);

    return function () {
        console.log("unsubscribed");
        clearTimeout(timer);
    };
});

thing.take(1).onValue(function (val) {
    console.log(val);
});

The unsubscribe callback is never called

According to this issue, the subscriber to 'thing' should detach automatically after 1 item, so maybe this is a bug with fromBinder?

Interestingly, if I change it to take(2), the callback is called

@myndzi
Copy link
Author

myndzi commented Jan 11, 2015

It appears to be the fault of the synchronous 'emit' -- if I make that asynchronous by wrapping it in process.nextTick(), it behaves as expected.

@rpominov
Copy link
Member

Yes, there is a bug. Not easy to fix on the first look though. Funny that I found similar bug in Bacon recently baconjs/bacon.js#523

The bug occurs when you truing to unsubscribe from a stream in response to the first value from it, and that first value is emitted synchronously.

Will think of how to fix this.

rpominov added a commit that referenced this issue Jan 11, 2015
* master:
  rm /dist
  0.5.2
  update changelog
  fix bug in .fromBinder when it not calls unsub with .take(1) and sync emit (fix #35)
@rpominov
Copy link
Member

The fix was actually pretty easy. Fixed in v0.5.2.

@myndzi
Copy link
Author

myndzi commented Jan 11, 2015

Cool, thanks!

Back to trying to get pagination to work

raimohanska pushed a commit to raimohanska/kefir that referenced this issue Jan 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants