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 feathers-hooks compatibility. #22

Merged
merged 3 commits into from
Nov 22, 2015
Merged

Conversation

marshallswain
Copy link
Member

This lets you define hooks directly in the entity object:

var authHooks = require('feathers-authentication').hooks;

var Property = {
  schema: {
    title: {type: String, required: true},
    description: {type: String},
    sold: {type: Boolean, 'default': false}
  },
  methods: {
    isComplete: function(){
      return this.complete;
    }
  },
  statics: {
  },
  virtuals: {
    'id': function(){
      return this._id.toHexString();
    }
  },
  indexes: [
  ],
  // Hooks
  before:{
    // These are just examples.  You are responsible to secure your own data.
    all: [authHooks.requireAuth],
    find: [authHooks.queryWithUserId],
    get: [authHooks.queryWithUserId],
    create: [authHooks.setUserId],
    update: [authHooks.setUserId],
    patch: [authHooks.setUserId],
    remove: [authHooks.setUserId]
  },
  after:{
    all: [],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
};

module.exports = Property;

This lets you define hooks directly in the entity object:

```js
var mongoose = require('mongoose');
var hooks = require('../hooks');

var Property = {
	schema: {
		title: {type: String, required: true},
		description: {type: String},
		sold: {type: Boolean, 'default': false}
	},
	methods: {
		isComplete: function(){
			return this.complete;
		}
	},
	statics: {
	},
	virtuals: {
		'id': function(){
			return this._id.toHexString();
		}
	},
	indexes: [
	],
	before:{
		find: [hooks.log]
	}
};

module.exports = Property;
```
@marshallswain
Copy link
Member Author

@ekryski I've run the tests locally and they pass when running a local mongod. I updated the test to show that adding before and after keys to the object does not interfere with the existing tests, but I'm not firing up a server to test the hooks.

What do you think? Can we merge?

screen shot 2015-10-03 at 4 50 30 am

@marshallswain
Copy link
Member Author

@daffl @ekryski Can somebody please comment on this?

@ekryski
Copy link
Member

ekryski commented Oct 26, 2015

@marshallswain Sorry for the delay mate. This kind of slipped passed me in my email. Thanks for pinging again!

My first thought was this makes things a bit more inconsistent with the other database adapters, but I think in this case that's ok. The mongoose adapter is a bit special. It works fine on my end so :shipit:.

Could you add some documentation to the readme about how to use them? I think you can just drop in your example.

To avoid confusion I think we should also highlight the difference between mongoose middleware and our hooks. A quick stab might be:

Feathers hooks are just middleware that are applied at the service level and give you more fine grained control over when each hook is applied by allowing you to specify which CRUD method will trigger them, as opposed to just being called on all saves or updates. With that said, both can be used in conjunction.

marshallswain added a commit that referenced this pull request Nov 22, 2015
Add feathers-hooks compatibility.
@marshallswain marshallswain merged commit 756fb8f into master Nov 22, 2015
@marshallswain marshallswain deleted the hooks-simple-setup branch November 22, 2015 02:09
@marshallswain
Copy link
Member Author

@daffl or @ekryski I've added the docs and merged. Can one of you publish this? Or add me to the npm package?

@daffl
Copy link
Member

daffl commented Nov 22, 2015

I've added you on NPM. Does this also address #25? If not we might want to add that as well before publishing. Side note: This plugin probably needs to be updated to the new structure, too.

@marshallswain
Copy link
Member Author

This doesn't fully address #25. Thanks.

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

Successfully merging this pull request may close these issues.

3 participants