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

socket.rooms is a useless object #2890

Closed
ki9us opened this issue Mar 8, 2017 · 8 comments
Closed

socket.rooms is a useless object #2890

ki9us opened this issue Mar 8, 2017 · 8 comments

Comments

@ki9us
Copy link

ki9us commented Mar 8, 2017

This is concerning the socket.rooms object, documented as:

A hash of strings identifying the rooms this client is in, indexed by room name.

I don't know what that means, but the example is clear:

io.on('connection', function(socket){
  socket.join('room 237', function(){
    console.log(socket.rooms); // [ <socket.id>, 'room 237' ]
    io.to('room 237', 'a new user has joined the room'); // broadcast to everyone in the room
  });
});

When I run console.log(socket.rooms);, I want that pretty array (so I can slice off <socket.id> and get just the rooms), but I get this ugly object back:

io.on('connection', function(socket){
  socket.join('room 237', function(){
    console.log(socket.rooms); // { <socket.id>: '<socket.id>', 'room 237': 'room 237' }
  });
});

... which I then have to turn into an array with .keys() and then pick out the socket.id by hand (since it might not be first).

Is this a bug? I'm using 1.7.3.

@darrachequesne
Copy link
Member

Well, it seems the documentation is not up-to-date here, the rooms was actually an array until v1.4 I think.

Note: I guess we could use a boolean as value.

@ki9us
Copy link
Author

ki9us commented Mar 9, 2017

I guess this is a feature request then. Make socket.io great again: bring back the arrays!

My goal here is to get an array of rooms the client is in. With an array, I assume the socket.id would be first:

var rooms = socket.rooms.slice(1);

Using the current implementation:

var roomKeys = Object.keys(socket.rooms);
var socketIdIndex = roomKeys.indexOf( socket.id );
var rooms = roomKeys.splice( socketIdIndex, 1 );

If the socket.id weren't included, it would be OK too. I just don't want to have to have to find it and pick it out.

var rooms = Object.keys(socket.rooms);

With booleans, I would be able to filter out the true values, like this (right?):

var roomKeys = Object.keys(socket.rooms);
var rooms = roomKeys.filter(function(key){ 
  return socket.rooms[key]==true;
});

Which is just as slow as iterating with indexOf().

It would be helpful to include rooms that the client isn't in (such as to show chat rooms it can join). I just think that information would be better elsewhere, like the Server instance:

var allRooms = io.rooms;
var myRooms = socket.rooms;

Anyone else have an opinion about this?

@chargingsleipnir
Copy link

Yah this seems unnecessarily complicated. I'm really struggling to understand why the user's socket id constitutes being a "room"?, or why key:value pairs are needed. It just needs to be a list - whatever rooms are present, you are in, done.

['room1', 'room2', 'room3']

And as above, io.rooms to get them all, either, also as a string array, or in this case as an object with room names as keys and an array of socket ids as their value.

{ room1: ['id1', 'id2', 'id3'], room2: ['id4', 'id5', 'id6'] }

@StefansArya
Copy link

I think the main reason is the lookup performance.
For the example, when a user was joined 100 room it would be more faster to use object lookup to find out if he was joined a room rather than iterating array value.

Refer to my answer on stackoverflow

{
    "room1":"room1",
    "room2":"room2",
    ...
    "room100":"room100"
}

To check if user has joined a room just a simple like this:
if(socket.rooms[roomID]) return true;

If it's array type then we need to use indexOf or iterate every array:
if(socket.rooms.indexOf(roomID) != -1) return true;

But I totally agree if the the key value is changed to a boolean:
{ room1: true, ... , room100: true }

It may help saving some memory usage

@scottzockoll
Copy link

I wouldn't mind changing socket.rooms so that the values are booleans. Is it okay to work on this? I'm not sure what the contributing process is as I didn't see a CONTRIBUTING.md file.

@CherryDT
Copy link

Wouldn't a Set make more sense?

if (socket.rooms.has(roomID)) return true;

The structure would be simply Set(4) {"<socket ID>", "room1", "room2", "room3"} for example

@scottzockoll
Copy link

scottzockoll commented Jun 14, 2020

I agree as long as the set's time complexity is constant. I believe in javascript it is.

@darrachequesne
Copy link
Member

In Socket.IO v3, Socket#rooms will now be a Set, and use the value provided by the underlying adapter, see here.

dzad pushed a commit to dzad/socket.io that referenced this issue May 29, 2023
The value stored in the adapter will now be used, instead of
duplicating it in the Socket class.

Breaking change: Socket#rooms is now a Set instead of an object

Closes socketio#2890
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

No branches or pull requests

6 participants