-
Notifications
You must be signed in to change notification settings - Fork 34
adds an initial state to the socket state #56
Conversation
| this.socket.close(1000, 'Closed normally.'); | ||
| this.queue.forEach(packet => packet.cancel()); | ||
| this.queue.clear(); | ||
| } |
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.
Probably want to set the state to Closed if the conditional fails, for cleanliness. Not sure if anything actually looks at it.
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.
sounds good ;)
ProbablePrime
left a comment
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.
What connor said
src/wire/Socket.ts
Outdated
| this.queue.forEach(packet => packet.cancel()); | ||
| this.queue.clear(); | ||
| } else { | ||
| this.state = SocketState.Closing; |
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.
Don't we mean "Closed" here, its not closing. its already closed?
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.
Websocket close is a three-way handshake, this starts it off but it doesn't complete until the close event is emitted.
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.
there's no 'Closed' state on the SocketState.
Idle, Connecting, Connected, Closing, Reconnecting, Refreshing
Happy to add a Closed state though :)
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.
Actually, well. It would make sense to transition back to Idle. Functionally closed and idle would be identical. Ignore my previous comment, then 😛
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.
Ahh understood, no this is fine as it is
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.
So we can proceed with this as is? Or should I revert back to just the socket state init change?
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.
Sorry, revert the conditional else. Then good to go
Adds initial socket state of 'idle',
Only close if not idle or not reconnecting.