search

nkzawa / socket.io-stream

Stream for Socket.IO
MIT License
611 stars 111 forks source link

socket.removeAllListeners() breaks ss(socket) #35

Open peteruithoven opened 9 years ago

peteruithoven commented 9 years ago

Currently when you call removeAllListeners on the socket connection that's used by socket.io-stream you break the socket.io-stream insance. Should we overrule the removeAllListeners method and make sure we re-add the socket.io-stream listeners? Something like: 

var originalRemoveAllListeners = sio.removeAllListeners;
  sio.removeAllListeners = function() {
  originalRemoveAllListeners.apply(sio, arguments);
  // re-add socket.io listeners
}
nkzawa commented 9 years ago

I think calling removeAllListeners without the target event can break socket of socket.io too. So I recommend to specify what to remove like:

socket.removeListener('myevent', listener);
socket.removeAllListener('myevent');
peteruithoven commented 9 years ago

I don't think removeAllListeners on socket.io breaks it. I use the general removeAllListeners to clean up after each (Mocha) unit test (in a general afterEach). This didn't break any socket.io sockets yet. It would be great if these cleanups could remain as generic as possible.

nkzawa commented 9 years ago

Actually, a socket listens an event at least internally like the following, even though removing the listener doesn't affect apparently.

https://github.com/Automattic/socket.io-client/blob/master/lib/manager.js#L301

anyway, I will think about solutions.

NicoleY77 commented 8 years ago

+1