-
Notifications
You must be signed in to change notification settings - Fork 405
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(websocket-plugin): get rid off rxjs/webSocket
and use WebSocket
directly
#2033
Conversation
b823b11
to
3562726
Compare
BundleMonUnchanged files (3)
No change in files bundle size Unchanged groups (3)
Final result: ✅ View report in BundleMon website ➡️ |
BundleMon (NGXS Plugins)Files updated (2)
Unchanged files (14)
Total files change +242B +0.41% Groups updated (3)
Final result: ❌ View report in BundleMon website ➡️ |
BundleMon (Integration Projects)Files updated (1)
Total files change -578B -0.88% Final result: ✅ View report in BundleMon website ➡️ |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 8fa8bf7. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 3 targetsSent with 💌 from NxCloud. |
b611209
to
91eb94a
Compare
91eb94a
to
815c37a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make sure that we test and handle correctly the scenario where the connect method is called after the service is destroyed.
815c37a
to
de852ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still prefer to have the _destroy$
changed to a ReplaySubject
just so that we are consistent with a recommended practice. But I'll leave it up to you on what you do.
Also, can you resolve the conflicts with master
?
I tried, but saw that it was reintroducing the initialiser code that you removed. Best that you do it 👍
…Socket` directly This commit updated the WebSocket handler implementation and switched to using the native `WebSocket` directly, instead of relying on `rxjs/webSocket`. The RxJS WebSocket implementation can be cumbersome when it comes to handling socket events and lacks simplicity. By handling socket events manually, we avoid the need to use the RxJS implementation.
de852ff
to
406f9bc
Compare
Code Climate has analyzed commit 8fa8bf7 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 86.4% (50% is the threshold). This pull request will bring the total coverage in the repository to 95.5% (0.0% change). View more on Code Climate. |
This commit updated the WebSocket handler implementation and switched to
using the native
WebSocket
directly, instead of relying onrxjs/webSocket
.The RxJS WebSocket implementation can be cumbersome when it comes to handling
socket events and lacks simplicity. By handling socket events manually, we avoid
the need to use the RxJS implementation.