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

Refactor/use observable instead of Subject for fromSubscribe in new-block-stream #112

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@adrianmcli
Copy link
Member

commented Jun 11, 2019

Thanks to @jklepatch for bringing this issue up with me. In #47, we left this out because we want to provide access to the underlying subscription. But since we can just give the user the ability to unsubscribe by returning the unsubscribe function inside the observer, perhaps it's better to use this method so we can simplify the API and remove the need for a cleanup function.

@adrianmcli adrianmcli requested a review from honestbonsai Jun 11, 2019

@adrianmcli

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

@honestbonsai I wanted to create this as a draft PR since I am not sure if we actually want this change quite yet. But I don't think this repo has the option for that (or I couldn't find it).

Even though the tests pass, the CI seems to show there's some unhandled promise rejection errors underneath. See here.

Not sure why that is, but that's making me want to hold off on merging this unless we sort things out.

@honestbonsai
Copy link
Contributor

left a comment

Docs need to be updated to reflect this change as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.