Skip to content

Conversation

scottinet
Copy link
Contributor

@scottinet scottinet commented Nov 28, 2017

⚠️ depends on #259 ⚠️

  • Simplify needlessly complicated Room object workflow by:
    • getting rid of the renew method (documentation update incoming shortly)
    • regrouping the active and subscribing state flags into an unique roomstate state machine
    • answering with errors and letting users control their workflow instead of trying to resolve problems they might not have in the first place
  • remove the unused options argument from Room.unsubscribe
  • fix event leaks occuring after multiple subscribes
  • fix https://github.com/kuzzleio/kuzzle-sdk/issues/53

@codecov-io
Copy link

codecov-io commented Nov 28, 2017

Codecov Report

Merging #260 into 6.x will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/Room.js 100% <100%> (ø) ⬆️
src/networkWrapper/protocols/abstract/realtime.js 98.63% <100%> (ø) ⬆️
src/Kuzzle.js 97.46% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad5e54f...9d2396a. Read the comment docs.

Copy link

@ballinette ballinette left a 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)

@scottinet
Copy link
Contributor Author

@ballinette > this should not happen, as the room is marked inactive after a network disconnection or after an authentication token has expired. In both these cases, Kuzzle will automatically remove the subscription and stop notifying that connection.

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.

@ballinette ballinette merged commit e55b85c into 6.x Dec 4, 2017
@ballinette ballinette deleted the remove_room_renew branch December 4, 2017 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants