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

Before adding a userhook to be loaded, check that it was not marked as 'not' to load via config #3546

Closed
wants to merge 2 commits into from

Conversation

luislobo
Copy link
Contributor

@luislobo luislobo commented Feb 4, 2016

It fixes https://github.com/balderdashy/sails-hook-sockets/issues/16, where the sails-hook-sockets is disabled via config, but since it is a module, it is being loaded via the userhook method

@luislobo luislobo closed this Feb 4, 2016
@luislobo luislobo reopened this Feb 4, 2016
@sgress454
Copy link
Member

Hi @luislobo -- thanks for tracking this down. I think what we need to do is patch the version in the stable branch with the fix that's already there on master.

@luislobo
Copy link
Contributor Author

luislobo commented Feb 5, 2016

well, yes, do you want me to just patch it? I mean, I generated the PR already... Sorry if I don't get your idea 😁

@luislobo
Copy link
Contributor Author

luislobo commented Feb 5, 2016

Oh, I see what you mean, put both conditions on the same line... right?

@luislobo
Copy link
Contributor Author

luislobo commented Feb 5, 2016

Well, I've checked and that fix actually is enough! I've compared both sails.config.hooks[hookName] and sails.hooks[hookName] and both contain the same value when setting sockets : false.
I think that then, this PR is not necessary.
We could change it to if (sails.config.hooks[hookName] === false || sails.hooks[hookName] === false) { to be very specific...
I've tested a new app using the current master branch and it does not fail.

@sgress454
Copy link
Member

@luislobo Sorry if I wasn't clear. This problem is already fixed on master; no patch is necessary. The issue balderdashy/sails-hook-sockets#16 is happening on the current stable version of Sails (v0.11.x), which can be patched by making a PR to the stable branch. It would be nice to have 0.11.x fixed as well, even though we're about to release 0.12.

@luislobo luislobo closed this Feb 5, 2016
@luislobo
Copy link
Contributor Author

luislobo commented Feb 5, 2016

New PR here: #3550 for current stable v0.11.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants