-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Removes the socket hook's dependency upon the session hook. #2274
Removes the socket hook's dependency upon the session hook. #2274
Conversation
Cool, thanks! |
No problem. Let me know if you've got any concerns. |
Can this be merged? |
@@ -19,6 +19,8 @@ module.exports = function(sails) { | |||
|
|||
return function socketAttemptingToConnect (handshake, accept) { | |||
|
|||
if(!sails.config.hooks.session) return accept(null, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- space after if.
- (My preference) add curly braces, always.
What is the hold up with this? |
@tarlepp working on it- affects the sio1.0 stuff |
👍 |
Any update on this pull request guys? |
@loicsaintroch @lwansbrough I think this is sorted in https://github.com/balderdashy/sails-hook-sockets/blob/master/lib/initialize.js#L50 - would you mind installing 0.11.0-rc5 and giving it a go? |
Thanks Mike, I'll take a look a little later. |
@lwansbrough alright I believe we're good now. Added tests here: https://github.com/balderdashy/sails-hook-sockets/blob/master/test/without-session.test.js Going to go ahead and close- thanks for your help :) |
@mikermcneil Oops forgot to report back! Works great thank you. |
@mikermcneil Does this require 0.11? |
@fabdrol yep it requires 0.11.x sails, I just tested this with sails 0.11.0 and session hook disabled and it works like a charm. |
So this removes the session hook requirements from the sockets hook. Helpful for those of us using JWTs.