feathersjs-ecosystem / feathers-mongoose

Easily create a Mongoose Service for Feathersjs.
MIT License
189 stars 96 forks source link

use .lean() liberally in fetch's? #51

Closed jamesjnadeau closed 8 years ago

jamesjnadeau commented 8 years ago

I would argue that a lot of the time, you don't need to fetch a full mongoose doc, and fetching a regular object without the mongoose overhead would be a great performance feature/optimization.

I think there should be a query param called $lean that defaults to being set to true.

When $lean is false, real mongoose objects are created, and this note would still actually apply: http://docs.feathersjs.com/databases/mongoose.html#modifying-results-with-the-toobject-hook

This way it would highlight what to do if you need to actually work with the mongoose object, but still provide the most performance by default.

Just an idea I had while looking through the source and from my own experience. Feel free to ignore, I won't have time/energy to work on anytime soon.

Thanks for this awesome project guys! Good work! :+1: :rocket: :guitar:

marshallswain commented 8 years ago

I find that with all of the functionality that hooks give you, the only feature of Mongoose that I actually use is the schema enforcement. It makes perfect sense for me to default to using lean.

daffl commented 8 years ago

Does that mean that you get faster fetches but the schema is still being enforced when someone creates a new instance? That sounds good to me.

ekryski commented 8 years ago

hmmmm. Not gonna lie my gut reaction was "just use the mongo adapter" (if it were up to date :hankey: :stuck_out_tongue_closed_eyes:) but I like this idea of defaulting to lean queries.

It looks like .lean() just prunes out all the virtuals, instance methods, static methods, etc. so it does make it quite a bit faster. My concern is are hooks enough to cover this, specifically in the case of virtuals?

I'm thinking that rather than passing on a per query basis you pass {lean: false} as an option when initializing a mongoose service instead.

I'm wrestling with the idea of this being a special query param. It's definitely more flexible this way but I think we really need to add special query params sparingly because if we introduce a bunch that are database specific it weakens our ability to query with a unified interface. Most of the database specific config options currently happen at initialization.

Do you guys think it's common that you'd have times where you'd want lean and not lean queries from a single service? cc/ @jamesjnadeau

Apologies for the long comment....

daffl commented 8 years ago

If we make { lean: true } an options flag (instead of the default) we don't even have to publish a new major version and it could go in fairly quick.

ekryski commented 8 years ago

Ya I actually like that better because then it is explicit that you are deviating from how mongoose behaves by default. Otherwise, I could see us getting questions about why virtuals and instance methods aren't available in hooks.

jamesjnadeau commented 8 years ago

:+1: to the options flag, better way of going about this. Doesn't really matter to me if it's on or off by default.

You can always call Model.hydrate(object) later to get it back to a mongoose doc if you need it.

For ease of use, might be best to leave the full mongoose docs there unless you choose this setting.

ekryski commented 8 years ago

@jamesjnadeau good to know. We should document that Model.hydrate call.