vti / protocol-websocket

Protocol::WebSocket
Other
40 stars 27 forks source link

Various improvements to Protocol::WebSocket::Client #61

Closed greg-kennedy closed 5 years ago

greg-kennedy commented 5 years ago

This request wraps up several changes to Protocol::WebSocket::Client to make it more friendly to end users.

All changes are supposed to be backwards compatible with previous users, but they add some convenience methods and safeguards against improper use. I have tested this with a custom driver program and it seems to work pretty well. However, I know there are dependencies (e.g. https://github.com/plicease/AnyEvent-WebSocket-Client) that rely on this module, and I haven't checked them thoroughly for issues.

vti commented 5 years ago

Thanks Greg, those are very useful changes. A couple of suggestions if I may?

Thanks :)

greg-kennedy commented 5 years ago

Well, don't mind the mess there... I was able to reduce it to five commits and fix those issues. I think it's ready to go : )

vti commented 5 years ago

Well...

t/client.t ........................... 1/? Protocol::WebSocket::Client: write() on unconnected WebSocket. at /home/vti/dev/protocol-websocket/lib/Protocol/WebSocket/Client.pm line 139.

    #   Failed test at t/client.t line 70.
    #          got: ''
    #     expected: '�'
    # Looks like you failed 1 test of 1.

#   Failed test 'write close frame on disconnect'
#   at t/client.t line 71.
Protocol::WebSocket::Client: write() on unconnected WebSocket. at /home/vti/dev/protocol-websocket/lib/Protocol/WebSocket/Client.pm line 139.

    #   Failed test at t/client.t line 81.
    #          got: ''
    #     expected: anything else
    # Looks like you failed 1 test of 1.

#   Failed test 'call on_write on write'
#   at t/client.t line 82.
# Looks like you failed 2 tests of 7.
vti commented 5 years ago

I guess they are failing because it now checks if it's connected or not.

vti commented 5 years ago

On disconnect the close frame is sent as masked, are you sure about that?

vti commented 5 years ago

Found this in the spec Close frames sent from client to server must be masked as per Section 5.3.

greg-kennedy commented 5 years ago

Yeah, I guess the tests are wrong, because it seems they are trying to do things (write, disconnect, etc) without an actual session in place.

I guess you could modify the test to futz with $client->{state} and set it to 1 before you do something else with it.

That said I will modify client.t and see where we can get.

vti commented 5 years ago

I already fixed the tests, just added a connect. I guess this is the correct way anyway. Thanks!