Don't delete ws when closing the server #444
Conversation
|
@perrin4869 thanks! Can you indeed add the test you're suggesting please? |
| @@ -191,7 +191,7 @@ Server.prototype.close = function () { | |||
| if (this.ws) { | |||
| debug('closing webSocketServer'); | |||
| this.ws.close(); | |||
| delete this.ws; | |||
| // don't delete this.ws because it can be used again if the server starts listening again | |||
There was a problem hiding this comment.
I understand the purpose here, but won't the comment seem a bit weird since the line delete this.ws; will be deleted?
There was a problem hiding this comment.
Yeah it's there in order for people not to add it again in the future, and give the reasoning why.
I tend to add this kind of comments since sometimes I forget the reason I removed a certain line and saves me the time in trying it again, but of course it can be removed. It'll be a bit redundant if we manage to test this
There was a problem hiding this comment.
At the end of the day, I think that, since deleting this.ws is such a tempting thing to do, the comment might help future contributors understand why we can't do it, and will save them time it would take them to figure it out on their own.
|
I could try to add the test, but I am not very well acquainted with |
|
Managed to add a test to prevent regression :) |
|
Maybe a similar test should be added to socket.io |
|
@perrin4869 thanks! |
The kind of change this PR does introduce
Fixes #442
Maybe a test to protect from regressions should be added, similar to what @veloce48 did in the original issue at the end.