feat: add client socket ID to LiveQuery ws_disconnect, ws_connect , connect and subscribe event
#7999
Conversation
Thanks for opening this pull request!
|
krtooch
changed the title
krtooch
changed the title
feat : (ParseLiveQuery) add client socket Id on ws_disconnect
krtooch
changed the title
feat : (ParseLiveQuery) add client socket Id on ws_disconnect
krtooch
changed the title
krtooch
changed the title
mtrezza
changed the title
ws_disconnect event
Codecov Report
@@ Coverage Diff @@
## alpha #7999 +/- ##
=======================================
Coverage 94.11% 94.11%
=======================================
Files 182 182
Lines 13621 13621
=======================================
+ Hits 12819 12820 +1
+ Misses 802 801 -1
Continue to review full report at Codecov.
|
|
@krtooch thanks for the PR, let us know when this is ready for review. |
|
ok for review. |
There was a problem hiding this comment.
Thanks for the PR and for taking this change into your own hands!!
I think it is expected that all onLiveQueryEvent triggers will work the same. I have noticed no other events have clientId. I am all for adding clientId but I think it should be added to ws_connect and the other events.
| if (event === 'ws_disconnect') { | ||
| Parse.Cloud._removeAllHooks(); | ||
| expect(sessionToken).toBeDefined(); | ||
| expect(clientId).toBeDefined(); |
There was a problem hiding this comment.
For completeness can we add tests for the other keys in req (e.g req.masterKey)
|
Hello i added clientId, to |
krtooch
changed the title
ws_disconnect eventws_disconnect, ws_connect , connect and subscribe event
| expect(useMasterKey).toBeDefined(); | ||
| expect(installationId).toBeDefined(); | ||
| } | ||
| if (event === 'subscribe') { |
There was a problem hiding this comment.
Could you split this up into individual tests for each event? Bulk tests have shown to be more difficult to maintain, also the test description would not be accurate anymore: "access user on onLiveQueryEvent disconnect".
|
@krtooch Could you address any open review comments, so we can merge this? |
New Pull Request Checklist
Issue Description
In order to create a Online/offline functionality with ParseLiveQuery socket protocol, i'd like to add client's socket Id to 'ws_disconnect' event, in order to know which socket is disconnecting.
Approach
Just add existing client ID, to ws_disconnect Object.
TODOs before merging
This is my first ever contribution on Github, so sorry if not totally well done.