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

Do (should) we manually have to handle `Bacon.noMore` inside `Bacon.fromBinder`? #484

Closed
mhelvens opened this Issue Dec 4, 2014 · 4 comments

Comments

Projects
None yet
2 participants
@mhelvens
Contributor

mhelvens commented Dec 4, 2014

If I'm reading the Bacon.fromBinder documentation correctly, the subscribe function is responsible for checking the return value of sink, and unsubscribing when Bacon.noMore is returned.

However, this is not done in the example code. Also, I imagine Bacon.js could handle this automatically by wrapping sink under the hood and conditionally calling the provided unsubscribe function (and then making sink a no-op to ignore any subsequent calls).

Not being fluent in Coffeescript, I'm not sure whether Bacon.js already does this. If so, consider this a documentation bug report. If not, consider this a feature request.

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska Dec 4, 2014

Contributor

It's true that subscribe should check for sink return value for Bacon.noMore. In most cases this doesn't really matter though: the internal Dispatcher mechanism will call the returned unsub function anyway. The only case where this would really hurt is a synchronously responding source, where a lot of (or infinite number of) events are pushed immediately in subscribe before the unsub is actually returned so that the Dispatcher can call it. In any case, the Dispatcher will shield the actual subscribers from unwanted values so the only nastiness caused by a missing noMore check is a possible loss of performance in pushing events synchronously.

So this should be considered a documentation bug report. There's a related blog post with source code on Github. This post could be used when improving the documetation.

Contributor

raimohanska commented Dec 4, 2014

It's true that subscribe should check for sink return value for Bacon.noMore. In most cases this doesn't really matter though: the internal Dispatcher mechanism will call the returned unsub function anyway. The only case where this would really hurt is a synchronously responding source, where a lot of (or infinite number of) events are pushed immediately in subscribe before the unsub is actually returned so that the Dispatcher can call it. In any case, the Dispatcher will shield the actual subscribers from unwanted values so the only nastiness caused by a missing noMore check is a possible loss of performance in pushing events synchronously.

So this should be considered a documentation bug report. There's a related blog post with source code on Github. This post could be used when improving the documetation.

@mhelvens

This comment has been minimized.

Show comment
Hide comment
@mhelvens

mhelvens Dec 4, 2014

Contributor

OK, that's clear. Thanks!

Contributor

mhelvens commented Dec 4, 2014

OK, that's clear. Thanks!

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska Dec 4, 2014

Contributor

Would you like to PR on improving the doc btw :)

Remember to see Contribution section in Readme

Contributor

raimohanska commented Dec 4, 2014

Would you like to PR on improving the doc btw :)

Remember to see Contribution section in Readme

@mhelvens

This comment has been minimized.

Show comment
Hide comment
@mhelvens

mhelvens Dec 4, 2014

Contributor

Done: #485

Contributor

mhelvens commented Dec 4, 2014

Done: #485

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