Skip to content
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

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

Merged
merged 2 commits into from Feb 26, 2021

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.

None yet

2 participants