-
Notifications
You must be signed in to change notification settings - Fork 25
handleUpgrade wasn't compatible with WS spec #12
Conversation
handleUpgrade wasn't compatible with WS module spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your pull request there are few things you need to change/clarify :). I can not see entry for upgradeHead
and callback
as handleUpgrade
is a private function and you need to pass this data in from some where.
lib/cws/server.ts
Outdated
@@ -26,6 +26,8 @@ export class WebSocketServer extends eventEmitter() { | |||
if (configs.path && configs.path[0] !== '/') { | |||
configs.path = `/${configs.path}`; | |||
} | |||
|
|||
this._upgradeCallback = noop; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename variables to just upgradeCallback
and move at the top with private tag (it is TypeScript not a JS so it has slightly different approach in creating variables) for reference check out upgradeListener
variable.
lib/cws/server.ts
Outdated
@@ -151,7 +154,7 @@ export class WebSocketServer extends eventEmitter() { | |||
return socket.end(`HTTP/1.1 ${code} ${name}\r\n\r\n`); | |||
} | |||
|
|||
private handleUpgrade(req: HTTP.IncomingMessage, socket: Socket): void { | |||
private handleUpgrade(req: HTTP.IncomingMessage, socket: Socket, upgradeHead, callback): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to specify types for upgradeHead
and callback
lib/cws/server.ts
Outdated
@@ -165,6 +168,7 @@ export class WebSocketServer extends eventEmitter() { | |||
if (!this.serverGroup) return; | |||
|
|||
this.upgradeReq = req; | |||
this._upgradeCallback = callback ? callback : noop; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably easier to write this._upgradeCallback = callback || noop;
@goriunov: is it allright now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleUpgrade
does not really works the same way as WS library has even after this changes as it is not exposed if we are adding this logic we will need to also add upgrade
event and hook up handleUpgrade
together. Plus rewrite handleUpgrade logic abit.
handleUpgrade wasn't compatible with WS module spec