-
Notifications
You must be signed in to change notification settings - Fork 637
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
Helper for applying multiple plugins #473
Comments
Worth checking the API/implementation of this lib for inspiration https://github.com/justinfagnani/mixwith.js If you go the direction you're hinting at, I'd request to at least let |
@devinivy That library was my inspiration. I originally thought of adding a helper like: class Person mixin(Model, Plugin1, Plugin2) {
} but then the user would need to require the The implementation would be as simple as this: class Model {
static plugin() {
return _.flatten(_.toArray(arguments)).reduce((Model, Plugin) => {
return Plugin(Model);
}, this);
}
} Of course it will always extend class Person extends Model.plugin(Plugin1, Plugin2) {
} class Person extends Model.plugin(Plugin1).plugin(Plugin2) {
} |
Wouldn't it be easier to have a callable api that just reassigns objection.plugin(Plugin1)
objection.plugin(Plugin2)
objection.plugin(Plugin3) |
@fl0w Nope, then we would lose part of the flexibility. After that you would have no way to access the original unmodified |
@koskimas understood. Storing the plugin/model chain layout wouldn't help either? objection.inspectPluginChain = {
original: Model,
'plugin-name-1': Plugin1,
'plugin-name-2': Plugin2
} Extending with intent is always an option but, correct me if I'm wrong, generally plugins would be used application wide? (considering soft-delete/case-conversion/auto-update-timestamps perhaps). edit I'm not criticising the provided solution, just exploring by poking. Ignore if I'm annoying. |
Probably, but nothing prevents you from doing objection.Model = objection.Model.plugin(Plugin1, Plugin2); |
Ah, got it. Thanks. That's clean! |
Would you prefer class BaseModel extends Plugin4(Plugin3(Plugin2({opt: 'foo'})(Plugin1(Model)))) {
} This seems cleaner to me: class BaseModel extends Model.mixin([
Plugin1,
Plugin2({opt: 'foo'}),
Plugin3,
Plugin4
]) {
} |
That's sort of reminiscent of a stack of decorators (visually). edit, sorry, I prefer "mixin" - though I don't have any strong feelings. Can the term "plugin" be used in different contexts? I think "mixin" is more specific (thus easier to document?). |
Yeah, mixin is what we are doing. I think that's a better name. I'll still document plugin usage using simple function calls (like it is explained today) so that people see what is going on. I'll only add a side note about the |
Maybe a simple compose helper like this one can be added for multiple plugins (which are called higher-order functions). It is widely used and tested by import { compose, Model } from 'objection'
class Animal extends Model {
static tableName = 'animals'
}
// Single higher-order function
export default timestamps({ type: 'timestamptz' })(Animal)
// Multiple higher-order functions
export default compose(
timestamps({ type: 'timestamptz' }),
softDeletes
)(Animal) Another way is to use decorators(babel stage 2 proposal), though I prefer @timestamps({ type: 'timestamp' })
@softDeletes
class Animal extends Model {
static tableName = 'animals'
} |
@vladshcherbin The problem with applying the mixings after extending the parent class is that the class name will be the name of the last applied plugin class. All stack traces containing model methods will have confusing class names. Otherwise you can already do that. I'm getting the feeling that no-one is especially thrilled about the import { mixin, Model } from 'objection'
class Animal extends mixin(Model, [Plugin1, Plugin2]) {
static tableName = 'animals'
} Or would you prefer no helpers at all? @vladshcherbin If we go down that road, I could also add a |
While it's not a big deal to me, I think it would be useful to have a very basic one in objection to reinforce the standard way to write an objection plugin. I think it would be positive for the objection ecosystem. Whether it's |
@koskimas maybe class name won't be wrong if higher-order function is defined like this one? function timestamps(options) {
return WrappedModel => (
class extends WrappedModel {
...
}
)
} |
Try this: class X {}
const f = (M) => class extends M {}
const Y = f(X);
console.log(Y.name); and you get |
@koskimas actually it's giving me Using babel with |
Node 7 gives an empty string and so does node 4. I doubt that 5,6 or 8 act any different. Babel is not 100% compatible in this case. We can't write features for Babel. They must be written against ecmascript standard and actual native node. |
@koskimas for me, babel is working exactly the same as node. Here is the same code with pure node 8: |
You're right. Node 8 does work like that. Older versions give the empty string. That's good news, but I suspect that many plugin developers give names to their plugin classes. Maybe we should add this to best practices and encourage people to leave the classes nameless. |
@koskimas yeah, this will be great. With latest node versions and babel you get nice things like Adding plugins also becomes a joy with higher-order functions and a combining helper like |
thank you very much. |
This can become a mess:
Should we add a helper like this:
Or would
mixin
be a better name for the method?Arrays would also be supported:
The text was updated successfully, but these errors were encountered: