hoaproject / Websocket

The Hoa\Websocket library.
https://hoa-project.net/
422 stars 75 forks source link

Fire close event before closing and on error #62

Closed ghost closed 7 years ago

ghost commented 8 years ago

Firing the close event before closing allows for the user to grab the connection's unique ID. This is useful if the user is keeping track of connection based states across the lifecycle of a connection (e.g. session data).

Fire the event on a protocol error as well before closing.

Hywan commented 8 years ago

Hi :-),

Sorry for the late reply. We were all in holidays or very busy. Thanks for the PR! If I remind correctly, there was a specific reason to fire the event after closing but I don't remember or maybe it does no longer matter. The only thing I see is this is dangerous to keep the connection state explosed to the user. It could close it before us and then corrupt the workflow. What do you think?

I assign @Metalaka to this PR. Please, tell me if it's fine for you.

Metalaka commented 8 years ago

Instead of modifying the close event behavior, would it not be better to add a close-before event ?

ghost commented 8 years ago

That's not a bad idea. Having a pre and post event could be good. Like closing and closed.

Sent from my iPhone

On May 21, 2016, at 1:23 PM, Metalaka notifications@github.com<mailto:notifications@github.com> wrote:

Instead of modifying the close event behavior, would it not be better to add a close-before event ?

You are receiving this because you authored the thread. Reply to this email directly or view it on GitHubhttps://github.com/hoaproject/Websocket/pull/62#issuecomment-220790056

Hywan commented 8 years ago

I like close-before. Or maybe before-close?

@jmdevince Can you try the implementation? Do you need help?

Metalaka commented 8 years ago

@Hywan I've chosen close-before because it is already used here : https://github.com/hoaproject/Event#events

Hywan commented 8 years ago

@Metalaka Perfect!

Hywan commented 8 years ago

@jmdevince ping?

Hywan commented 8 years ago

@Metalaka What is the state of this PR?