search

celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
595 stars 87 forks source link

remove rack dep, add fwd to ping #189

Closed kenichi closed 9 years ago

kenichi commented 9 years ago

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 commented 9 years ago

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

jcoglan commented 9 years ago

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 commented 9 years ago

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:

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

kenichi commented 9 years ago

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

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

jcoglan commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

k, this should be ready to go then.

digitalextremist commented 9 years ago

Will test a couple things and cut tonight!