Skip to content

Fix #65: allow excluding all sockets in a room#66

Merged
darrachequesne merged 2 commits intosocketio:masterfrom
sebamarynissen:feature/exclude-rooms
Feb 26, 2021
Merged

Fix #65: allow excluding all sockets in a room#66
darrachequesne merged 2 commits intosocketio:masterfrom
sebamarynissen:feature/exclude-rooms

Conversation

@sebamarynissen
Copy link
Contributor

This PR adds a fix for #65 which allows excluding all sockets in a specific room when broadcasting.

@darrachequesne
Copy link
Member

Hi! Thanks for the pull request.

If I understand correctly, this will allow to do:

io.except(<room>).emit(/* ... */);
socket.broadcast.except(<room>).emit(/* ... */);

I'm wondering if it would make sense to also add:

io.to(<room1>).except(<room2>).emit(/* ... */);
socket.to(<room1>).except(<room2>).emit(/* ... */);

What's your opinion on this?

Related: socketio/socket.io#3657

@sebamarynissen
Copy link
Contributor Author

sebamarynissen commented Feb 2, 2021

I did not add an .except() method to the namespace and socket classes yet, but indeed that would be possible. I can add it and file a PR for this in https://github.com/socketio/socket.io if you'd like. I didn't add it because in my use case I'm doing some wizardry where I directly call adapter.broadcast() instead of using io.to(). It will definitely make it more useful though to explicitly add an .except() method to the namespace and socket classes too.

As for the io.to(<room1>).except(<room2>).emit(/* ... */) case, I did think about that as well and it should be possible by just moving the code I added out of the else statement. l did not do it because I thought it might be too confusing to mix too many rooms (as you'd be able to go real exotic like io.to(...).to(...).except(...).to(...).except(...)). However, the more I think of it, the more I am convinced that it would be better to allow it though.

Furthermore, I think that this functionality should be added to socket.io-emitter as well. I already created an issue for this a while ago (socketio/socket.io-redis-emitter#87), but I'll file a PR for that as well. It's just a matter of adding an .except() method.

As a conclusion, I propose that I do the following, if you agree:

@darrachequesne
Copy link
Member

Sounds good to me 👍

@sebamarynissen
Copy link
Contributor Author

@darrachequesne I've created all required pull requests in their respective repositories. I should note that I would still like to add tests for excluding specific rooms as well, but for that to work socket.io first needs to use an adapter containing this functionality of course.

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.

2 participants