On close passes incorrect data

[Joeao]

onClose (@:1157) Currently passes "transport close". The server expects an object with the property "room", containing the room name.

[Joeao]

Socket.emit('disconnect') is already called by Socket.io when closing the window, so it may be better to emit yjsDisconnect or similar as to not cause errors.

[Joeao]

A good argument towards changing the emit name would be for when we want to disconnect from a YJS instance and not trigger a whole socket connection disconnect.

[dmonad]

What exactly do you mean?

Calling Socket.disconnect('disconnect') shouldn't be a problem. I just tested it, you can disconnect one socket.io instance, and the other connections will remain.

Did you specify "room"? You have to in the current implementation. If you don't want that, you can just ignore this property.

Can you give me steps on how to reproduce your error?

[dmonad]

If you need to have a connection to the server after disconnecting a yjs instance. You should just maintain another socket.io-client instance (it will automatically use the existing tcp connection). I think this is the best way to go

[Joeao]

I guess the disconnect issue is actually caused due to me running just one socket server as Socket.io is a core component of my app. When disconnect is called, other functions are carried out which are unrelated to YJS. A YJS instance disconnecting is quite different from the overall socket instance being closed, in my case. I'm running all clients through the same socket server as it gives me access to socket session storage without having to rebind elsewhere.

As far as room goes, I am giving the room a name but a string is sent to the server rather than an object.

// Client side
socket.emit('disconnect', 'transport close');

// Server side
socket.on('disconnect', function(msg) {
    getInstanceOfY(msg.room) // msg.room does not exist, msg is a string
});

I should have split these two issues up really as the first (emit('disconnect')) is debatable, whereas msg.room missing seems more like an actual bug.

[Joeao]

Changing the socket.emit message still won't work if Socket.io is used elsewhere on the site. Socket.io's default msg on disconnect will override YJS's.

[dmonad]

I think I fixed the server side for you. I should not throw errors anymore when msg.room does not exist. But I still don't understand the client side bug (maybe because I'm unfamiliar with socket.io)

I have to call socket.disconnect(), because it's really the only way how to disconnect from the server. When a Yjs instance is destroyed, I don't need that connection anymore. I tested it, and this does not affect the other instances of yjs (they are still connected to the same server).

Do you have a proposal on how to do that better?

[Joeao]

Could you link me to where msg.room is set and where .emit('disconnect') is being triggered? It's likely that I'm looking in the wrong place.