Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

remove rack dep, add fwd to ping #189

Merged
merged 3 commits into from
Apr 11, 2015

Conversation

kenichi
Copy link
Member

@kenichi kenichi commented Apr 9, 2015

this should resolve #186 and #187 - in my testing, this limited amount of rack mimicry works, no need for a full gem dependency and Rack::MockRequest instantiation.

also, this adds a forward to the driver for WebSocket#ping.

@tarcieri
Copy link
Member

tarcieri commented Apr 9, 2015

Oops, sorry @jcoglan, the real PR is over here!

@jcoglan
Copy link

jcoglan commented Apr 9, 2015

I might be missing something but I think you need to map more headers than are in RACK_HEADERS. https://github.com/faye/websocket-driver-ruby#usage lists env parameters used by the WebSocket protocol.

@kenichi
Copy link
Member Author

kenichi commented Apr 9, 2015

Oh, I did leave out the older protocol keys. See the thread on the original websocket-driver integration. Reel has already parsed the headers, which is why Driver.rack is being called instead of Driver.server. Reel has already decided that:

  • it's a websocket ('Connection' & 'Upgrade')
  • it knows the url of the request ('Host')

I'll add the 'Origin', and key{1,2} headers to the map to support the older browsers.

@kenichi
Copy link
Member Author

kenichi commented Apr 9, 2015

Should 'Sec-WebSocket-Extensions' be on that list?

https://github.com/faye/websocket-driver-ruby/blob/master/lib/websocket/driver/hybi.rb#L241

@jcoglan
Copy link

jcoglan commented Apr 9, 2015

@kenichi Yes, it should. Personally, I would just map all the headers into the Rack env format so as not to encode knowledge about the WebSocket protocol into Reel, but if you would rather be explicit then you need all the listed keys to support older versions. If you only want to support the RFC version then what you have looks fine to me.

@kenichi
Copy link
Member Author

kenichi commented Apr 9, 2015

@jcoglan thanks, I added the header keys to support the older versions. Reel::Request::Info already knows a little bit about the protocol... I could be convinced about key translation. My main goal was remove the rack gem dependency from reel. Thoughts, @tarcieri @digitalextremist?

@tarcieri
Copy link
Member

tarcieri commented Apr 9, 2015

I really don't care beyond removing the rack dependency. Whatever works. Supporting older browsers would be nice if it's not too much extra work.

@kenichi
Copy link
Member Author

kenichi commented Apr 9, 2015

k, this should be ready to go then.

@digitalextremist
Copy link
Member

Will test a couple things and cut tonight!

digitalextremist pushed a commit that referenced this pull request Apr 11, 2015
remove rack dep, add fwd to ping closes #186
@digitalextremist digitalextremist merged commit 858c028 into celluloid:0.6.0-milestone Apr 11, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants