Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a middleware for promoting a connection to WebSocket (fix #205) #206

Closed
wants to merge 3 commits into from

Conversation

@arteymix
Copy link
Member

arteymix commented Feb 25, 2017

No description provided.

@codecov-io
Copy link

codecov-io commented Feb 25, 2017

Codecov Report

No coverage uploaded for pull request base (master@97f3792). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #206   +/-   ##
========================================
  Coverage          ?   62.3%           
========================================
  Files             ?      40           
  Lines             ?    1369           
  Branches          ?     170           
========================================
  Hits              ?     853           
  Misses            ?     448           
  Partials          ?      68
Impacted Files Coverage Δ
src/vsgi/vsgi-request.vala 67.37% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97f3792...7360801. Read the comment docs.

@arteymix arteymix force-pushed the wip/websocket branch from faa4b6a to 005d526 Feb 25, 2017
@ZanderBrown
Copy link

ZanderBrown commented Jan 14, 2018

Any chance of resurrecting this?

@arteymix
Copy link
Member Author

arteymix commented Jan 15, 2018

I'll look into it tomorrow.

@arteymix arteymix closed this Jan 15, 2018
@arteymix arteymix reopened this Jan 15, 2018
@arteymix arteymix force-pushed the wip/websocket branch from 005d526 to 6dfaddd Jan 15, 2018
@arteymix
Copy link
Member Author

arteymix commented Jan 15, 2018

I just tested the handshake so far, but I'll write a unit test using the client that comes with libsoup-2.4.

@ZanderBrown
Copy link

ZanderBrown commented Jan 15, 2018

Looking good, been wanting to have a serious play with valum for a while and I now find myself needing a WebSocket server. Having looked at the insanity of doing that in PHP it seemed as good a time as any to finally get valum out.

@ZanderBrown
Copy link

ZanderBrown commented Jan 28, 2018

Anything I could do to push this over the line?

@arteymix arteymix force-pushed the wip/websocket branch from 6dfaddd to ee35ed7 Jan 28, 2018
@arteymix
Copy link
Member Author

arteymix commented Jan 30, 2018

I finally managed to make it work! I'll push the testcase in a few hours (I have a course now).

Call 'next' if the client does not want to establish a handshake.

Add some documentation to cover the WebSocket middleware.
@arteymix arteymix force-pushed the wip/websocket branch from ee35ed7 to 269646a Jan 31, 2018
@arteymix
Copy link
Member Author

arteymix commented Jan 31, 2018

I wrote the test case against the example, but I'll complete it later. Meanwhile, you can test it and give it some feedback.

I'll try to fix the CI..

@ZanderBrown
Copy link

ZanderBrown commented Feb 1, 2018

Seem to have run into some problems that seem to be around valum expecting http:// but (at least in js) only ws://(or wss://) is valid for WebSocket connections.

Also ninja seems to timeout on the websocket test and my simple soup client reports

The server did not accept the WebSocket handshake.
@ZanderBrown
Copy link

ZanderBrown commented Feb 1, 2018

Okay pulled latest commit, now I get

ERROR:websocket-test.vala:18:__lambda5_: code should not be reached

For the test

@arteymix
Copy link
Member Author

arteymix commented Feb 1, 2018

You need to start the example application to run the test until I include it in the unit test itself. I think I'll use a fork for starting the server part.

@arteymix
Copy link
Member Author

arteymix commented Feb 1, 2018

I think I've found why it was not working, the Connection header can contains a param list.

I'll release a 0.4 with WebSocket support along with VSGI 1.0 soon after this get merged. I'd like to make it to the next TechEmpower benchmark!

@arteymix arteymix force-pushed the wip/websocket branch from 269646a to 7360801 Feb 1, 2018
@ZanderBrown
Copy link

ZanderBrown commented Feb 1, 2018

Connecting to examples/app /websocket in JS seems to work for me now

@arteymix
Copy link
Member Author

arteymix commented Feb 1, 2018

It was fixed in cea368d

Since it's working, I've merged the work into the trunk and I'll include the testcase later.

@arteymix arteymix closed this Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.