-
Notifications
You must be signed in to change notification settings - Fork 17
Add typescript support realtime #537
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
Conversation
Codecov Report
@@ Coverage Diff @@
## 7-dev #537 +/- ##
==========================================
- Coverage 88.50% 88.34% -0.17%
==========================================
Files 32 32
Lines 1427 1441 +14
Branches 242 249 +7
==========================================
+ Hits 1263 1273 +10
- Misses 117 119 +2
- Partials 47 49 +2
Continue to review full report at Codecov.
|
src/controllers/Realtime.ts
Outdated
* | ||
* Should be called on network error or disconnection | ||
*/ | ||
disconnected () { |
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.
🤔 can't we make this private/protected, since it's an internal method? Oh, it probably needs to be called by the Protocol class so that might be a reason to leave it public.
Ok, so we have something similar to the "loose-coupling" problem we had in the core.
By the way, the naming of this method looks confusing to me. If it was a handler, I'd call it onDisconnected()
, which tells that "what goes in this block gets executed when the connection is closed", which is OK, but I'd prefer the name of the method tells what the method does, i.e. "delete all the subscriptions of this instance of the SDK".
I posted a similar comment on a PR in the core, so I'd like to leave my 2 cents on the naming of this method (if it's not a breaking change)
- deleteAllSubscriptions()
- deleteAllRooms()
👋
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 take advantage of existing events and I renamed the methods to saveSubscriptions
and resubscribe
export enum ENotificationType { | ||
document = 'document', | ||
user = 'user', | ||
TokenExpired = 'TokenExpired' |
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.
Why PascalCase?
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.
Because Kuzzle sent the event like this :(
# [7.4.0](https://github.com/kuzzleio/sdk-javascript/releases/tag/7.4.0) (2020-08-18) #### New features - [ [#542](#542) ] Finalize typescript support ([Aschen](https://github.com/Aschen)) - [ [#529](#529) ] Add typescript support for protocols ([Aschen](https://github.com/Aschen)) #### Enhancements - [ [#541](#541) ] Add meaningful stacktrace ([Aschen](https://github.com/Aschen)) - [ [#537](#537) ] Add typescript support realtime ([Aschen](https://github.com/Aschen)) - [ [#531](#531) ] Add typescript support for Index and Collection controllers ([Aschen](https://github.com/Aschen)) - [ [#530](#530) ] Add typescript support for search results ([Aschen](https://github.com/Aschen)) - [ [#523](#523) ] Add support for the collection:delete API action ([morgandruesne](https://github.com/morgandruesne)) ---
What does this PR do?
Convert realtime controller to TS