Skip to content

Conversation

@overflowz
Copy link
Contributor

Please, check carefully before merging, because I'm not sure if I did it right. Thanks!

index.js Outdated
var socket = this.nsp.connected[request.sid];
if (!socket) { return; }

function sendAck(){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sendAck function is useless here, isn't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my comment wasn't clear enough! What I thought of was:

case requestTypes.remoteDisconnect:

  var socket = this.nsp.connected[request.sid];
  if (!socket) { return; }
 
  socket.disconnect(request.close);
  // the acknowledgement is still needed
  var response = JSON.stringify({
    requestid: request.requestid
  });
  pub.publish(self.responseChannel, response);
  break;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, could you please check it again?

@darrachequesne
Copy link
Member

Looks good to me! Could you please add a test, and a line in the README?

@overflowz
Copy link
Contributor Author

I'm kinda new to git(hub) but I'll try.

overflowz and others added 3 commits January 16, 2017 00:55
send acknowledgement in remoteDisconnect call.
@darrachequesne darrachequesne merged commit 11ab62b into socketio:master Jan 15, 2017
@darrachequesne
Copy link
Member

Thanks for that pull request! I added a test, to prevent future regressions.

@overflowz
Copy link
Contributor Author

You're welcome!

@darrachequesne darrachequesne added this to the 3.1.0 milestone Jan 16, 2017
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.

2 participants