-
Notifications
You must be signed in to change notification settings - Fork 17
Remove room renew #260
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
Remove room renew #260
Conversation
Codecov Report
@@ Coverage Diff @@
## 6.x #260 +/- ##
==========================================
- Coverage 98.27% 98.25% -0.02%
==========================================
Files 17 17
Lines 2142 2125 -17
Branches 620 618 -2
==========================================
- Hits 2105 2088 -17
Misses 37 37
Continue to review full report at Codecov.
|
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.
nitpicking: what do we do if the client still receive notifications while beeing inactive (I guess it shouldn't, but...) ?
I suggest, in notify
method, to emit the notification only if this.roomstate === 'active'
(and at least log a warning message if not)
@ballinette > this should not happen, as the room is marked I suggest to leave it like that: either it works as I think it will, or it does not and issues will be written, allowing us to detect a potential room management problem, probably on Kuzzle's side. |
Room
object workflow by:renew
method (documentation update incoming shortly)active
andsubscribing
state flags into an uniqueroomstate
state machineoptions
argument fromRoom.unsubscribe