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.

Sign up for GitHub

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

Jump to bottom

Using Subjects as Observers #825

Closed
staltz opened this issue Nov 29, 2015 · 11 comments
Closed

Using Subjects as Observers #825

staltz opened this issue Nov 29, 2015 · 11 comments
Labels
bug Confirmed bug

Comments

@staltz
Copy link
Member

staltz commented Nov 29, 2015

I think this is a bug. Currently RxJS 5 Subjects cannot be used as Observers in source.subscribe(subject), even though Subject implements the Observer interface.

// Rx 5
observable.subscribe(
  subject.next.bind(subject),
  subject.error.bind(subject),
  subject.complete.bind(subject)
)

//Rx 4
observable.subscribe(subject.asObserver())

This should be simple to fix, but should be accompanied by tests too.
@benlesh
Copy link
Member

benlesh commented Nov 29, 2015

This is totally a bug. They should work as observers. It's half the point.

@benlesh benlesh added the bug Confirmed bug label Nov 29, 2015
@benlesh
Copy link
Member

benlesh commented Nov 29, 2015

Also, you shouldn't have to use asObserver.

@staltz
Copy link
Member Author

staltz commented Nov 30, 2015

Agreed.

staltz added a commit to staltz/RxJSNext that referenced this issue Nov 30, 2015
@staltz
test(Subject): add tests for subjects used as Observer
2f7db2f 
Add tests for Subject, BehaviorSubject and ReplaySubject to check whether they can be used as plain
observers when given to Observable.subscribe().

Related to issue ReactiveX#825.
@staltz staltz mentioned this issue Nov 30, 2015
Closed
@staltz
Copy link
Member Author

staltz commented Nov 30, 2015

@Blesh Apparently I'm not able to reproduce the bug, it seems everything is ok. See PR #828. I'm awaiting for a bug report from @TylorS who originally reported this bug to me.

kwonoj pushed a commit that referenced this issue Nov 30, 2015
@staltz @kwonoj
test(Subject): add tests for subjects used as Observer
3c72bca 
Add tests for Subject, BehaviorSubject and ReplaySubject to check whether they can be used as plain
observers when given to Observable.subscribe().

Related to issue #825.
@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

@staltz ... I did fix a bug semi-related to this a while ago: #724

That might have been the culprit... I don't know.

@ntilwalli
Copy link
Contributor

@staltz Isn't this similar to issue #748?

@benlesh
Copy link
Member

benlesh commented Dec 1, 2015

@staltz @ntilwalli I just tried every Subject type from the latest build and I couldn't reproduce this issue.

If we can't reproduce it, can we close this?

@staltz
Copy link
Member Author

staltz commented Dec 2, 2015

Yeah let's close, Tylor couldn't reproduce it either.

@ntilwalli
Copy link
Contributor

This actually did not fix the issue from #748. I've created a pull request with a fix for that issue. #863

@benlesh
Copy link
Member

benlesh commented Dec 2, 2015

@ntilwalli is right: There is a minor issue that Subjects are being unnecessarily wrapped in Subscribers. This is less of a "bug" and more of a "refactor".

@ntilwalli ntilwalli mentioned this issue Dec 3, 2015
Closed
@kwonoj kwonoj closed this as completed in 5cb0f2b Dec 3, 2015
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Confirmed bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(Observable): Subjects no longer wrapped in Subscriber
3 participants
@staltz @benlesh @ntilwalli