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

WebSocketSubject seemingly ignores destination argument #1784

Closed
deontologician opened this issue Jun 25, 2016 · 6 comments
Closed

WebSocketSubject seemingly ignores destination argument #1784

deontologician opened this issue Jun 25, 2016 · 6 comments

Comments

@deontologician
Copy link
Contributor

deontologician commented Jun 25, 2016

RxJS version:
master branch

Code to reproduce:
https://github.com/ReactiveX/rxjs/blob/master/src/observable/dom/WebSocketSubject.ts#L67
Expected behavior:
The second argument to the constructor would be used somewhere
Actual behavior:
It is not used
Additional information:

I'm thinking the ReplaySubject should be subscribed to the destination argument being passed in so it gets its values.

@benlesh
Copy link
Member

benlesh commented Jun 27, 2016

@deontologician this is a bit of a tricky one. The "destination" of the WebSocketSubject is the "observer side" of the subject. @trxcllnt and I are doing some fancy things here using ReplaySubject to queue messages that need to be sent over the WebSocket when it opens. There really isn't a reason a user would need to set the destination of this particular subject, so we're just leveraging it.

It's definitely confusing though. haha

:)

Does that satisfy your issue? Is there a reason you'd like to use destination in WebSocketSubject?

@deontologician
Copy link
Contributor Author

Ah, I thought it might be used to queue up a handshake message before any other requests were sent. So should the destination argument be removed from the constructor?

@benlesh
Copy link
Member

benlesh commented Jun 30, 2016

@deontologician I trust this issue has been satisfied then? I'll close it for now, feel free to reopen if you still have questions or you feel there is still an issue.

@benlesh benlesh closed this as completed Jun 30, 2016
benlesh pushed a commit that referenced this issue Jun 30, 2016
@trxcllnt
Copy link
Member

@Blesh what's the status on merging #1790?

@deontologician
Copy link
Contributor Author

This issue was just why the destination argument was not being used at all. It looks like #1790 fixed it, so I'm good

@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.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants