Hook result data shouldn't have sequelize info

[RickEyre]

When using hooks on sequelize services hook.result has a lot of extra properties on it then just the data. This makes modifying the result data confusing.

[marshallswain]

This is the expected result of using an ORM. That data can be really useful in other scenarios, so it's generally better to use Sequelize's methods to convert the model instance to an object. See here for ways of doing this:

http://stackoverflow.com/questions/21961818/sequelize-convert-entity-to-plain-object

[marshallswain]

It's probably worth noting that you would do this in an after hook.

[RickEyre]

Hm, I can see what you're saying. I was just thinking it's inconsistent with the regular service hooks. In some cases you're doing hook.result.myProp = 'myProp'; in others you're doing hook.result.dataValues.myProp = 'myProp';, or even worse for an array (I think it's something like) hook.result.dataValues[0].dataValues.myProp = 'myprop'.

At some point those dataValues are converted to the format that you would normally expect. What if it happened earlier in the stack? Right before the after hooks. If we really want that ORM information for other purposes then why not put it in another property on the hook?

[ekryski]

@RickEyre for now we leave that up to you because it's not one size fits all. The Sequelize models that get returned have some model properties that come in handy in certain instances, like @marshallswain said. If we are to pluck them from the model and put them as other attributes on hook.result you would lose the individual model context, especially when dealing with multiple models that are returned, as is the case with find queries or batch creates, patches, and removes.

With that said, we have bundled hooks with some of the modules and have proposed in #18 and #19 to have similar behavior to the feathers-mongoose adapter, so that you have a hook that will call toJSON for you.

[ekryski]

@RickEyre I just generated a new app and took a closer look. I get what you are saying but my previous statement still stands. I think #18 and #19 should solve the problem quite nicely, if you want your results converted to regular objects right away, as opposed to having sequelize models returned.

I'm going to close this as we can track progress in those 2 issues.

[RickEyre]

Sounds good! Thanks for pointing me in the direction of those issues.

[ekryski]

@RickEyre if you want to implement the hook yourself in the mean time, it's pretty much exactly the same as this one: https://github.com/feathersjs/feathers-mongoose/blob/master/src/hooks.js. PR's are even better! 😄

[RickEyre]

@ekryski Yeah, I was thinking of taking a crack at it 😄 I'll see if I can find some time in the next few days. Doesn't look that difficult!

[bedeoverend]

@RickEyre @ekryski would using the sequelize model instance's built in toJSON function do the job? Working well for me, though haven't tested thoroughly.

[petermikitsh]

Sequelize (unfortunately) wraps results by default in DAO's. You can disable them everywhere edit: almost everywhere -- see below with {raw: true}.

About to go through my project and do that everywhere, in fact.

Not that feathers-sequelize should necessarily be opinionated about it, but there are noted performance benefits for disabling DAO's, particularly with complex queries, or queries that return large data sets.

[ekryski]

@petermikitsh @bedeoverend we have been talking about doing a breaking change to both feathers-sequelize and feathers-mongoose so that services are initialized with {raw: true} and {lean: true} respectively. @daffl saw some pretty massive performance improvements with using lean vs. without.

[petermikitsh]

I support that effort. Note that setting {raw: true} won't turn off DAO's for all Sequelize API calls. See my writeup at sequelize/sequelize#6501 for more details regarding the Sequelize Model.create api. It's a bit of a gotcha. 😕