feathersjs-ecosystem / feathers-hooks-common

Useful hooks for use with FeathersJS services.
https://hooks-common.feathersjs.com
MIT License
193 stars 90 forks source link

include does not do a deep merge with its results #132

Closed Morriz closed 7 years ago

Morriz commented 7 years ago

instead it uses Object.assign.apply, which supposedly only does shallow copying. (see lib/populate.js:123)

This screws with the results of the included arrays with complex objects like from mongoose. I suggest something like lodash.merge is used instead.

Morriz commented 7 years ago

After more thorough inspection I found it is due to merging with a mongoose result object, which has setters that cast any other mongoose objects to just their ObjectID. So if the merge is done on an empty object this is prevented. So line 123 should become:

return Object.assign({}, item, ..._toConsumableArray(children));

But then the new generated prop is not added to the model._doc value array. Pffffffff

Morriz commented 7 years ago

If I bring in some mongoose awareness I can show it is solved like this:

return Object.assign(item.$toObject ? item.$toObject() : item, ..._toConsumableArray(children));

So can we somehow normalize the objects depending on the adapter used?

Morriz commented 7 years ago

I say we can set an options.normalize function which can be passed in the hooks populate(... call:

return Object.assign(options.normalize ? options.normalize(item) : item, ..._toConsumableArray(children));

That way we can spec a normalize function per service, which is usually tied to one db or another, or none.

eddyystop commented 7 years ago

Forgive my ignorance, but wouldn't this be avoided if lean: true was used? No hook works correctly with Mongoose ORMs.

On Tue, Feb 28, 2017 at 6:13 AM, Maurice Faber notifications@github.com wrote:

I say we can set an options.normalize function which can be passed in the hooks populate(... call:

return Object.assign(options.normalize ? options.normalize(item) : item, ..._toConsumableArray(children));

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/feathersjs/feathers-hooks-common/issues/132#issuecomment-283012273, or mute the thread https://github.com/notifications/unsubscribe-auth/ABezn28moocBboJviZfZpxXwZIUMx5TUks5rhAF2gaJpZM4MOJOY .

eddyystop commented 7 years ago

There's been no response in 22 days, so I'm closing the issue. Feel free to reopen it with more info.