Skip to content

Feature/connected voters count#151

Merged
perkynades merged 17 commits intodevelopfrom
feature/connected-voters-count
Apr 13, 2021
Merged

Feature/connected voters count#151
perkynades merged 17 commits intodevelopfrom
feature/connected-voters-count

Conversation

@perkynades
Copy link
Copy Markdown
Contributor

I have no idea if this is the best way to do this, but this seems to work, any comments will be very helpfull :D

Comment thread src/services/SocketRoomService.ts Outdated
@perkynades perkynades requested a review from freshfish70 April 8, 2021 11:40
Copy link
Copy Markdown
Member

@freshfish70 freshfish70 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you also implement the leave function to be called?

Comment thread src/services/SocketRoomService.ts
Comment thread src/services/SocketRoomService.ts Outdated
Comment thread src/services/SocketRoomService.ts Outdated
@perkynades
Copy link
Copy Markdown
Contributor Author

Have we implemented the voter leaving an election? The reasoning for me leaving the TODO is because i thought that the "socket" logic for this wasnt implemented yet

@freshfish70
Copy link
Copy Markdown
Member

If we consider the disconnect event.

Disconnection
Upon disconnection, sockets leave all the channels they were part of automatically, and no special teardown is needed on your part.

So we dont have to make them leave the room if they disconnect. But if they are able to stay on same same socket connection and join another election, we have to implement room unsubscribing.

@perkynades
Copy link
Copy Markdown
Contributor Author

If we consider the disconnect event.

Disconnection
Upon disconnection, sockets leave all the channels they were part of automatically, and no special teardown is needed on your part.

So we dont have to make them leave the room if they disconnect. But if they are able to stay on same same socket connection and join another election, we have to implement room unsubscribing.

Ok, we should discuss this since it is out of the scope for this issue :P. But i will rename the method

@freshfish70
Copy link
Copy Markdown
Member

:D I was not indicating that you should implement anything, hehe. Just removed the todo.
But i think you should implement the event that updates connected count when voter disconnects :D that is in scope of the issue.

@perkynades
Copy link
Copy Markdown
Contributor Author

:D I was not indicating that you should implement anything, hehe. Just removed the todo.
But i think you should implement the event that updates connected count when voter disconnects :D that is in scope of the issue.

Ohh sorry now i see what you meant, yeah the event is wrong, good spot! :)

Copy link
Copy Markdown
Contributor

@sanderhurlen sanderhurlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, peltoninho 👯 You and @freshfish70 have full control on this one 💪🏼

Comment thread src/services/SocketRoomService.ts Outdated
Comment thread src/services/SocketRoomService.ts Outdated
Copy link
Copy Markdown
Contributor

@spiftire spiftire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions

if (decoded) {
if (decoded.organizer) {
organizerJoin({ ...event, data: decoded })
event.acknowledgement(EventMessage({}))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be empty?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know, maby @freshfish70 knows :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes 👌🤩
This is for acking the joining, and We dont have any data to send back on Success full join. We Just call it to Notify frontend. The empty bracket is Just for having a default value.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. Maybe leave a comment for future ourselves

const connectedVoters = socketServer.of('/').adapter.rooms.get(electionId.toString())?.size

const electionRoom = this.getRoom(electionId)
if (electionRoom) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there is no electionRoom?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it will return undefined, leading to the stuff inside the if-statement not happening

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should maby add an exception, so that it will not continue with the emit

@perkynades perkynades merged commit d474f2e into develop Apr 13, 2021
@perkynades perkynades deleted the feature/connected-voters-count branch April 13, 2021 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants