Closed kacperk closed 10 years ago
I'm reviewing this now - n.b. that per https://github.com/sockjs/sockjs-client this is the current ruby server version now. I'm updating the README to say so in the course of this merge.
Thanks for this - hopefully it was less pain for you than for me to get the gem working in the first place!
First problem I'm having is that all the EM specs are hanging for me - I'm not sure if this has to do with this PR or updates to other gems in the interim. I'm investigating that.
Reviewing the code, I have some comments, which I'm addressing at the moment...
About the specs - for me they were taking around 15 minutes to run, but all were passing.
Okay - I'll give them a while to run. There's a reason they're tagged :em => true, if I recall correctly.
On Fri, Dec 6, 2013 at 11:41 AM, Kacper Kawecki notifications@github.comwrote:
About the specs - for me they were taking around 15 minutes to run, but all were passing.
— Reply to this email directly or view it on GitHubhttps://github.com/nyarly/sockjs-ruby/pull/7#issuecomment-30022893 .
@kacperk Have you had a chance to make any progress on this? I'll likely merge this locally and complete the refactor myself tonight or tomorrow. Regardless, thanks for your hard work.
@nyarly unfortunately I have a lot of work now, so I won't be able to start working on this before friday.
Probably it will be good to merge it to original request but I forked from you, so it have to be merged to your fork before.
There are some bug fixes (taken also from other forks) Protocole updated to 0.3.4 -- i followed the changes from socks-node repo and made them in ruby. Updated to use faye-websocket 0.7.1 Added closed hook to session.
All test are passing. Also tested manually with thin.
it should also work with more servers then thin, but I haven't test it yet.