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

add "hookName" option to installable hook config #3168

Merged
merged 1 commit into from
Aug 20, 2015

Conversation

tjwebb
Copy link
Contributor

@tjwebb tjwebb commented Aug 20, 2015

  • cleaned up formatting
  • set "hookName" in order to specify installable hook name.
    e.g.
{
  "sails": {
    "isHook": true,
    "hookName": "myHook"
  }
}

- set "hookName" in order to specify installable hook name.
e.g. {
  "sails": {
    "isHook": true,
    "hookName": "myHook"
  }
}
tjwebb added a commit that referenced this pull request Aug 20, 2015
add "hookName" option to installable hook config
@tjwebb tjwebb merged commit aa8f54f into balderdashy:master Aug 20, 2015
@sgress454
Copy link
Member

Won't this allow two NPM-published hooks to have conflicting names?

@tjwebb
Copy link
Contributor Author

tjwebb commented Aug 29, 2015

Currently, sails treats the hooks sails-hook-foo and foo identically. The hookName option gives the hook an opportunity to de-clutter the namespace and avoid conflicts with other hooks.

@sgress454
Copy link
Member

Gotcha--yeah the idea was to make the sails-hook- prefix the best practice for creating Sails hooks (that way you can easily find them on Github or npmjs.org). What you did is fine, but it could catch someone off-guard if they install sails-hook-email and some-awesome-email-hook and the latter sets its hook name to email (causing a conflict). Either way, you can change the hook name on a per-app basis using sails.config.installedHooks['some-awesome-email-hook'].name--probably should document that somewhere...

@tjwebb
Copy link
Contributor Author

tjwebb commented Aug 30, 2015

Yea, a lot of the docs around hooks isn't as good as it could be. I'd like to work on improving this as well.

As far as naming: I'd like to adopt the strategy of http://react-components.com, where they mine npm for a react-component keyword rather than relying on a naming prefix. npm now supports namespaces, so @orgA/sails-hook-foo would be the same as @orgB/sails-hook-foo. So my assertion is that naming conflicts are already possible, though as a practical matter, not really a burning issue. The hookName feature at least gives hook the opportunity to resolve these conflicts.

Another point is that I'd like hooks to be thought more of as "extensions" or "plugins". That's what they are, but cramming everything into the "hook" category doesn't quite capture the breadth of possibility. sails-permissions for example is technically a "hook", but it's functionality reaches far beyond that of a typical hook: it installs policies and models, it augments existing models, and so forth.

@sgress454
Copy link
Member

probably should document that somewhere...

I stand corrected, it's here

Yea, a lot of the docs around hooks isn't as good as it could be. I'd like to work on improving this as well.

Knock yourself out! :)

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.

2 participants