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

Guidance on improving WebSocketSubject multiplex method #2766

Closed
HipsterZipster opened this issue Jul 25, 2017 · 2 comments
Closed

Guidance on improving WebSocketSubject multiplex method #2766

HipsterZipster opened this issue Jul 25, 2017 · 2 comments

Comments

@HipsterZipster
Copy link

RxJS version: 5.4.2

Code to reproduce: No bug to reproduce, just a question.

Expected behavior:

Actual behavior:

Additional information:
On Line 122 of WebSocketSubject it says "// TODO: factor this out to be a proper Operator/Subscriber implementation and eliminate closures"

Can one of the maintainers please explain a little more clearly what's the suggested improvement and the benefits it would bring? I'd love to contribute, but am a little confused what we're looking for.
I've noticed @benlesh authored https://github.com/benlesh/RxSocketSubject/blob/master/test/RxSocketSubject/multiplex.js awhile back for RxJS 4, are we looking to take a similar approach here as well?

Thanks

@benlesh
Copy link
Member

benlesh commented Jul 25, 2017

Honestly, at this point we can probably remove that TODO. I initially added that comment because there was a huge performance difference in Chrome/V8 between code that used a lot of closures and code that did not. Since then the gap there has tightened enough to where I'm less worried about it.

@HipsterZipster
Copy link
Author

Thanks for responding @benlesh. If someone were to further build out this WebSocketSubject multiplex functionality with more features like the ability for the consumer to send or configurable auto-retry (Likely not belonging in the core rxjs repo, but I’m open to anything), can you opine what your ideal approach would be?
Maybe I’d create an operator that returns a multicasted subject? (Multicasted so that multiple downstream subscribers don’t trigger repeated subscription msgs as the current one does)

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