Make WebSocketServer#handleUpgrade() callback optional

[lpinca]

If the upgrade is successful and the callback argument is not
provided, then emit the 'connection' event.

Refs: #1966

[lpinca]

I don't like this because the method behavior depends on the callback argument. Instead it should always or never emit the 'connection' event, regardless of whether the callback argument is given or not.

[sdrsdr]

Works for me! 😃 This implementation is better but deeper. I as "external" contributor was striving for minimal code impact with my approach.

[sdrsdr]

https://github.com/websockets/ws/blob/5b87c5191ddbdae974f553fa841270b7919ae10c/test/handleupgrade-default-cb.test.js this is a test case I've forgotten in my firts commit in my PR. You can use it to test your implementation also

[lpinca]

https://github.com/websockets/ws/blob/5b87c5191ddbdae974f553fa841270b7919ae10c/test/handleupgrade-default-cb.test.js this is a test case I've forgotten in my firts commit in my PR. You can use it to test your implementation also

It's already covered by this change

ws/lib/websocket-server.js

Line 108 in bc73223

upgrade: this.handleUpgrade.bind(this)

A lot of existing tests would fail if the 'connection' event is not emitted.

[lpinca]

I will keep this open for a while to collect feedback. If no one will comment I will close it.

[sdrsdr]

If you publish new package version I can try update the TypeScript definitions at DefinitelyTyped

[sdrsdr]

What is the timeframe for this PR?

[lpinca]

See #1968 (comment). Let's wait a few more days.

[lpinca]

I'm closing this as per #1968 (comment).

[sdrsdr]

Closed but not merged? I'm not sure what's going on here!

[lpinca]

Yes, aside from yours, there seems to be no interest in getting this merged.