First of all, I want to say that ES6 class models is the amazing thing, which allows covering mongoose models with static analysis and auto-completion with Flowtype/Typescript. An example with flowtype. Thanks a lot for this ability!
ES6 class models
covers static and regular methods, virtuals fields, BUT do not provide any ability to define pre
& post
hooks.
@vkarpov15 how do you think is there a way to add hooks to the class models somehow like this?
These pre/post
hooks in models do not require parallel execution or several serial. It can be done via Promise.all
or several functions calls inside this methods. Main aim to provide access to hooks via model class definition with static analysis.
class User {
preSave() {
this.touchedAt = new Date();
}
postSave() {
// In `post` hooks changed access to document via `this`
// instance scope. (was `doc` arg)
// It's done due better static analysis.
console.log(this.id);
}
preInit() {}
postInit() {}
preValidate() {}
postValidate() {}
preRemove() {}
postRemove() {}
}
class User {
static preInsertMany() {
// `this.` is binded to model context and methods.
}
static postInsertMany() {};
}
Via static properties, cause methods have query
context in this
. So better do not use regular instance/static method for this kind of hooks. For them can be used preQueryHooks
and postQueryHooks
static properties.
class User {
static preQueryHooks = {
count: function() {
// `this.` is binded to query context
},
find: function() {},
findOne: function() {},
findOneAndRemove: function() {},
findOneAndUpdate: function() {},
update: function() {},
};
static postQueryHooks = {
count: function() {},
find: function() {},
findOne: function() {},
findOneAndRemove: function() {},
findOneAndUpdate: function() {},
update: function() {},
};
// ----- OR -------- (suppose that this is better)
static queryHooks = {
preCount: function() {},
postCount: function() {},
// etc.
}
}
Thoughts?
This can already mostly be achieved with real class inheritance. For example:
/* @flow */
import mongoose, { Model, Schema } from 'mongoose';
import argon2 from 'argon2';
const schema = new Schema(
{
email: {
type: String,
required: true,
index: true,
},
password: {
type: String,
required: true,
hidden: true,
},
}
);
// real inheritance
class UserModel extends Model {
email: string;
password: string;
createdAt: string;
updatedAt: string;
// ...
async encryptPassword(password: string): Promise<string> {
return argon2.hash(password);
}
// pre/post-save hook works
async save(options, next) {
// Hash the password
if (this.isModified('password')) {
this.password = await this.encryptPassword(this.password);
}
// call actual save
await super.save(next).then(doc => { console.log(doc); /* post hook */ });
}
}
export default mongoose.model(UserModel, schema);
@nodkz btw, it would be nice, if this syntax could be added to the flow-types. I didn't find time to prepare a pull-request.
I like this syntax a lot better and I'm unsure why the hacky loadClass-syntax is promoted via the docs. Are there any reasons for this?
@benbender that syntax feels cleaner, but both of these syntaxes still make it unintuitive to add multiple save hooks (like for plugin support). Any suggestions on how we might do that with a class-based syntax?
@vkarpov15 I don't think there is a clean solution. If you go for class-inheritance, you have a hierarchical model where posthooks (worse: multiple ones!) don't quite fit.
If I had to decide, I would probably go for a clean hook-api outside of the model-class hierarchy, which is exposed by the model, to register those hooks. In this case, the execution of those hooks would be the job of the mongoose core and not the model. I think this would be the only way of a clean separation of concerns - at least as far as I've digged into mongoose.
quick example:
// real inheritance
class UserModel extends Model {
email: string;
password: string;
constructor() {
this.registerHook('preSave', async () => {
// Hash the password
if (this.isModified('password')) {
this.password = await this.encryptPassword(this.password);
}
});
this.registerHook('postSave', () => { ... });
// ...
}
// ...
async encryptPassword(password: string) {
return argon2.hash(password);
}
}
@benbender Thanks for your solution (first one), it looks very promising 鉂わ笍. But I tried, and it does not work for me with post hooks:
async save(options, next) {
// never finished, and `super.save` does not return promise
const res = super.save(next);
// or with `options` save record properly, but again does not return promise
const res = super.save(options, next);
console.log(options, next, res); // [Function: fnWrapper] undefined undefined
}
Any advise how force super.save
to return promise?
but both of these syntaxes still make it unintuitive to add multiple save hooks (like for plugin support). Any suggestions on how we might do that with a class-based syntax?
@vkarpov15 how I know plugins attach to Schema, not Model. Here in this issue we discuss definition of model via class. If I miss, that model also can have its own plugins, maybe we can use something like this:
class User extends Model {
static plugins = [Plugin1, Plugin2];
}
But it's ugly, better to keep plugins only on schema level. And this.registerHook
in constructor (second @benbende example) also not good solution. I think if a developer wants to apply several plugins on model level, I suppose, better somehow to call them explicitly in save
method (first @benbende example):
class User extends Model {
async save(opts) {
// pre hooks code
const res = await super.save(opts);
// post hooks code
return res;
}
}
Or with inheritance:
class ModelExtended extends Model {
async save(opts) {
// pre hooks code 1
const res = await super.save(opts);
// post hooks code 1
return res;
}
}
class User extends ModelExtended {
async save(opts) {
// pre hooks code 2
const res = await super.save(opts);
// post hooks code 2
return res;
}
}
// on User.save() we get following calls:
// pre hooks code 2
// pre hooks code 1
// save
// post hooks code 1
// post hooks code 2
If somebody wants to call some hooks in parallel, it can use Promise.all
syntax:
class User extends Model {
async save(opts) {
await Promise.all([ asyncHook1, asyncHook2, ... ]);
const res = await super.save(opts);
// post hooks code
return res;
}
}
Honestly current synchronious/parallel hooks syntax scary me 馃
Anyway while super.save
does not return Promise we cannot keep going with real class inheritance
.
Really it will be very cool if pre/post hooks will be removed via class inheritance.
class MyModel extends Model {
await save() {
await preSaveHooks;
return super.save().then(() => postSaveHooks);
}
await init() {
await preInitHooks;
return super.init().then(() => postInitHooks);
}
await validate() {
await preValidateHooks;
return super.validate().then(() => postValidateHooks);
}
await remove() {
await preRemoveHooks;
return super.remove().then(() => postRemoveHooks);
}
static async insertMany() {
await preInsertManyHooks;
return super.insertMany().then(() => postInsertManyHooks);
}
// and so on
}
That unfortunately won't work because of plugins. Feels too much like fitting a square peg into a round hole, OOP isn't the right solution for everything. For one thing, we keep hooks in schemas rather than models for performance reasons, so we can build the function once when the model is compiled.
Most helpful comment
This can already mostly be achieved with real class inheritance. For example:
@nodkz btw, it would be nice, if this syntax could be added to the flow-types. I didn't find time to prepare a pull-request.
I like this syntax a lot better and I'm unsure why the hacky loadClass-syntax is promoted via the docs. Are there any reasons for this?