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

Jump to bottom

Add headers parameter for HTTP::WebSocket constructors #4227

Merged
merged 1 commit into from Jun 26, 2017
Merged

Add headers parameter for HTTP::WebSocket constructors #4227

merged 1 commit into from Jun 26, 2017

Conversation

adamtrilling
Copy link
Contributor

@adamtrilling adamtrilling commented Apr 1, 2017 •
edited

Added a headers parameter to both constructors for HTTP::WebSocket, to be used as follows:

HTTP::WebSocket.new(URI.parse("ws://websocket.example.com/chat"),
  HTTP::Headers{"Authorization" => "Bearer authtoken"})

Now you can pass headers along with your websocket request, for authorization and whatnot. I've created an issue for this (#4222) but it was so easy to implement that I just went ahead and did it.

@bcardiff
Copy link
Member

bcardiff commented Apr 3, 2017

So, this PR allows to send headers while creating a websocket when using crystal as client.

It does not cover accessing headers in the websocket in http server, since the headers are not accessible from a HTTP::WebSocket AFAIK and there is no change in the line where a websocket is created for the server side counter part: https://github.com/crystal-lang/crystal/blob/master/src/http/server/handlers/websocket_handler.cr#L33

Although I see value in supporting in a nicer way both headers and querystring, I think it's support needs to arrive in both sides of the http protocol at the same side.

@asterite
Copy link
Member

asterite commented Jun 4, 2017

I would merge this right now. This lets you open a websocket connection with headers specified, regardless of whether you are going to provide a websocket server, so it's useful by itself.

@RX14
Copy link
Contributor

RX14 commented Jun 26, 2017

soo, can we merge this?

@mverzilli mverzilli merged commit 11b1e3c into crystal-lang:master Jun 26, 2017
@mverzilli
Copy link

Thanks @adamtrilling !

@mverzilli mverzilli added this to the 0.23.0 milestone Jun 26, 2017
@RX14
Copy link
Contributor

RX14 commented Jun 26, 2017 •
edited

@bcardiff Actually, doesn't WebSocketHandler take a block with arguments for both the websocket and the HTTP::Server::Context, which would allow you to access the request headers.

@sdogruyol sdogruyol mentioned this pull request Sep 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
kind:feature topic:stdlib
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@adamtrilling @bcardiff @asterite @RX14 @mverzilli