Basic plugin architecture

[fabien]

Similar to http://mongoosejs.com/docs/plugins.html

Next, loopback-boot should be updated to support loading plugins from dirs.

[fabien]

I found myself mixing in functionality all the time, and this provides a lightweight and flexible way to do so. There should probably be some common core plugins added to ./plugins. Additionally, we should see if the before/after hooks can be made more flexible - monkey-patching them in plugins is probably not the best way to go forward, but it works for now.

@raymondfeng @ritch looking forward to your input - I'm itching to tackle loopback-boot integration.

[raymondfeng]

@fabien Great stuff. Thanks! I have a few questions/suggestions:

1. We have the mixin capability which allows a model class to mix in other model classes, see https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/model.js#L389. To some extent, it is a plugin defined in the form of abstract model class. I guess we should converge both forms.
2. What about we call the plugin as trait or mixin?

[fabien]

@raymondfeng I know about mixins, they seem to be a tad more involved but less generic and thus less flexible. In my plugin method I can do - dynamically - whatever I want, not just extend/mixin. And there's options to configure the plugin at runtime.

However, I'd be all for covering both concepts under the mixin umbrella. A file loaded from such a /mixins directory could be either a lightweight callback (~currently: plugin) or an abstract model class (~mixin). We should easily be able to detect both styles, and act accordingly. What do you think?

Also, any ideas on fixing the hook limitation of one method/fn to be defined, without breaking BC?

[raymondfeng]

Cool, +1 to support both styles.

A mixin can be defined as:

• A abstract model class or corresponding LDL (json) definition
• A custom function that is responsible for extending/configuring a model

A mixin can be loaded from:

• A local module loaded from mixins directory
• A npm module that can be required

BTW, we're working on improving hooks so that they will be close to middleware styles and should allow chaining.

[fabien]

@raymondfeng regarding: A abstract model class or corresponding LDL (json) definition

Could you post a gist of how those 2 definitions would look? I'm not sure how the model class is set up - which dependencies to require to create such a model in code in a simple manner. It seems a bit cumbersome for this purpose.

[raymondfeng]

Here is what I have in mind:

LDL format:

{
  name: 'MyModel',
  properties: {
     ...
  }
  mixins: ['TimeStamp', 'MyMixIn']
}

Abstract model class:

MyModel.mixin(TimeStamp, MyMixIn, ...);

[fabien]

That's not how these are defined in a module, right? It looks to me that's how you'd actually apply - mix-in - them. I was referring to the code in such modules, or in the .json LDL for the mixin definition (like MyMixin) itself.

Also, in LDL I'd prefer an object over an array, to be able to pass in an options object. Do you see a need to 'classify' the mixin names (as loaded from the module file name)?

{
  name: 'MyModel',
  properties: {
     ...
  }
  mixins: { TimeStamp: { foo: 'bar' }, MyMixin: true }
}

[fabien]

Looking at http://docs.strongloop.com/display/LB/LoopBack+Definition+Language#LoopBackDefinitionLanguage-Mixinginmodeldefinitions it appears that at a minimum, you'd need a modelBuilder instance to declare such a mixin in code. For little gains over the LDL in JSON.

Therefor, I think we should support these 3 types of mixins:

1. a module function, taking the arguments: Model, options (most flexible)
2. a module object, to directly extend the Model.prototype (any object properties, functions)
3. an LDL definition in JSON (simple setup)

[fabien]

@raymondfeng I implemented 1, 2 and 3 but it looks like there's a bug in the 'old' mixin implementation (currently still unresolved) - see the commented-out tests, where a LDL-defined mixin fails to actually mix in the properties. Unless this is delayed or implemented differently, I expected the properties to be hard-wired after applying the mixin.

[fabien]

@raymondfeng I updated loopback and loopback-boot to support mixin loading.

[offlinehacker]

I agree this should be also avalible outside loopback-boot

[raymondfeng]

If we step back, the mixin function is basically a hook to the model definition. I would like to generalize the idea so that we don't have to introduce too many concepts/layouts.

A few opinions/questions:

1. Can we implement the mixin function as follows?
   mixin-1.js (dropped into boot/)

module.exports = function(app) {
  app.models.forEach(function(model) {
    ...
  });
}

1. Please note juggler needs to be compatible with browsers. Using file system apis make it hard.
2. What about leaving the registry in juggler and have loopback-boot to discover such JS files as mixins?

[offlinehacker]

Makes sense, i will give 1. a try, because i really need this functionality

[offlinehacker]

Well there's one problem, how to support input parameters and to which models mixins apply using boot.

[raymondfeng]

Where are the input params from? Can you give me one example?

For the models that need the mixins, we should be able to configure them via model definitions.

Simply speaking, we need to have a way to discover a bunch of JS files and invoke them following a method signature as the contract. boot supports two flavors today (models and boot). We can improve them based on your use cases.

[fabien]

@raymondfeng please keep the current implementation as much as possible, there are reasons for doing this having the option to call mixin() explicitly (ex: User.mixin()) - some plugins should be called at runtime at different stages than model definition or during boot.

And mixins should definitely accept options/params, not just from LDL, but also at runtime mixin(), for example to supply functions/callbacks (which cannot be defined in json LDL).

Regarding 1. you can always use mixins.define() to define mixins in-code (on the browser side). 2. ties loading of mixins totally to loopback(-boot), and cripples the ability to use mixins seperately. How else would they be of any use to juggler, if there's no way to load them (other than to define them)?

[fabien]

@raymondfeng re: For the models that need the mixins, we should be able to configure them via model definitions.

To me mixins are vastly different from the mixins than the ones outlined here: http://docs.strongloop.com/display/LB/LoopBack+Definition+Language#LoopBackDefinitionLanguage-Mixinginmodeldefinitions - this is just some syntactic-sugar to me.

I don't think these are useful at all - zero use-case for me, because of their inherent static nature. For real-world usage, a plugin/mixin should be dynamic - see Mongoose's vast library of plugins of what you're able to do. I was hoping to bring this to loopback's juggler, and perhaps offer some useful core mixins as part of the module itself (hence the loading code here, instead of elsewhere).

Not to mention, as I pointed out before, I don't think they actually work as expected (see #issuecomment-51322687 above).

[raymondfeng]

@fabien Maybe what you have in mind for the mixin function is broader than I think. Let's first make sure if that's the case.

1. The mixin function is responsible for further configuring/extending an existing model (defined by code or LDL) to whatever way it wants. The method signature is: mixin(Model, options);
2. You develop the JS function and export it.
3. You drop in the .js files into somewhere so that the system can discover them and add them to a registry by names. (The discovery should be optional and there should be an api in juggler to register mixin functions by name).
4. Now models can apply the mixin by name, like what is documented at http://mongoosejs.com/docs/plugins.html. For example, MyModel.mixin(tsMininFunc).

[fabien]

@raymondfeng what you describe is exactly what I implemented AFAIK, even the api to register functions by name.

Is this what you have in mind as well? Or is this where our thoughts diverge? Note that the old-style mixin functionality (as outlined in the docs) is also supported, but even now I'm not sure if that code actually works (the commented-out tests I was referring to above).

Also, as you can see, I'm using the mixin functions to internally implement the old-style extend-functionality, as well as object-based extend, which keeps the code concise.

[offlinehacker]

I had some problems if i defined mixins in model json file, more
specifficialy when i was overriding deleteById function as part of mixin.
It worked if i defined mixin in model js file.

On Fri, Aug 8, 2014 at 12:03 AM, Fabien Franzen [email protected]
wrote:

@raymondfeng https://github.com/raymondfeng what you describe is
exactly what I implemented AFAIK, even the api to register functions by
name.

Is this what you have in mind as well? Or is this where our thoughts
diverge? Note that the old-style mixin functionality (as outlined in the
docs) is also supported, but even now I'm not sure if that code actually
works (the commented-out tests I was referring to above).

—
Reply to this email directly or view it on GitHub
#201 (comment)
.

-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: GnuPG v1.4.11 (GNU/Linux)

mQENBFEY1PEBCADPOfERF2wo4qeoq9L1m2z4pKfWqNd4B6BsoFUWPNd7BXmY+9JG
jJddSkmYobWec7XjAFTBL0Xbttt+rK9SIED2dCOmU1FYMQElhGlM3PNA3kaiQFeV
ijgH318GCfZzDd0dWa5TN/IshVeWXwgngsIEmZTVf1VSeb3eO3B8Fxe3zsSLUq0b
71MmU5eLVP9pMkm5V5BTYp+lV70FIekKygkKq+uTDo1csWUatbs4Qvgv37Bymy2t
oTwOBXGoinQk5N/6asR1jWs3vKv0L0SruoZy/kEG/jXb4l2OZUP85EVMganYKouE
OchVmcmhBdWV+t3HK4r2ATfyEcMRzvzSflA1ABEBAAG0Jkpha2EgSHVkb2tsaW4g
PGpha2FodWRva2xpbkBnbWFpbC5jb20+iQE+BBMBAgAoBQJRGNTxAhsDBQkB4TOA
BgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIXgAAKCRD6Zxi5hZclKnXNCACLOKa8abQp
eTWv9SXUwC7LVM5pP2mXcgn+Ipqr6YWBdLx4Iij0YlvUok9VeKvwTpUlT+cx++o3
wCM3AYrUyJE+zrtw49lInUmutz9seqJLU895oq+D+UuGoORrLBpEZrYR5f83uUmQ
E3Z1ZmWrNGXYtITWDtVZD/KauMF2nkPcmy/XaYXhd4WHD81DGNlKtGAHig6A3Phc
8Mr0A4yLDeRQJm8lCFEsxMJUNTupgY+ybbsMfVGx1gQvvGOTioV8CLCoRchUCCcm
YPArFg40KzIDSNjwdo9EVZDnlPx1hbOppfQydxP+JVnZsqoYmVY4UhIWi/NfOl3V
UMjl338INW1zuQENBFEY1PEBCADRSIfelOMjaTH7IfpMFFUc5Gys//njFnW9QAUg
wyfs2AFxUp6vKQ7nxXQiJXVhKTwe9iqo+oGxaHp4AeTjC7vXsfMuF5g5lfttbAo3
YEobEe6OG5so41nbwan6SyeIIQ2AmQqJBw8TKKMSec2qUN0Pw7iZRs0o9uJM/obG
DPsAsMOQgNLxJyMCP7X2jBtDXxkMFVHMmk50Tl3h3Fi9qWuNxgTXjs0tUvKkXiu2
Pco952jnm7HpCIKBek2pqR/UJXXb5qxy5G6Lc0qaMWZ5GKnSMTJmTY6Xl44EnaLK
zh0rqgF9qpoWck470ZbiGASMtB008hy2l0cyxUfvDaS3tY4hABEBAAGJASUEGAEC
AA8FAlEY1PECGwwFCQHhM4AACgkQ+mcYuYWXJSoT6AgAkvzvC0EGmeCR3cn9O3Gf
yG00Kqk9/1gJvlphis7AAce8iUgU+4xd94Vp0u8rghpdy88xKN5lF1W2YZQmmBaf
AVe6b7TOg6kxc3GKkVsWDxNyQKkpB46BwefIGaSljH7502X9aEWosrqWyJJNYCtt
QDit4BysX0Ww3Ka5Rx6ZFhC9ybPKoW2i8JwpyBaXDt7R2k+PC/ClBf9qzL+sb2es
zh/zCMVKNdm8KUITHU/5lgn2qZpUFZwiASPCMGGFP9u8g6UKeUTYTPD+GWaHIW63
RAgNIAffxx0M1r3P/2ipkAdI3NX/1iBKDQNG8Odsf+BswFKrNCnyUDdLPvJAhODS
gw==
=tmrm
-----END PGP PUBLIC KEY BLOCK-----

[raymondfeng]

@fabien I think we are mostly on the same page. The only difference in opinions is that I think the discovery and loading of mixin functions should live outside of the juggler. To use juggler as a library without loopback, the mixin js files/npm modules can be required and registered. Or the hosting app can provide their own convention for auto discovery and registration.

The old style of mixin is just a special case of mixin function. It can be viewed that there is a built-in mixin function that can take another model and add all properties/methods to the target model.

[fabien]

@offlinehacker my guess is that mixins defined in json/LDL are applied at a slightly different stage - methods like deleteById are probably mixed in by juggler itself after that. If so, we should find a better point (~event) in the life-cycle of a model to apply mixins. @raymondfeng thoughts on that?

@raymondfeng re: loading of mixin functions - I don't see this as a big deal, those < 40 LOC make no assumptions about the LB app-structure whatsoever, it is very generic, while still auto-handling the different types of mixins (LDL/JSON, Object, Function). Without it, you'd have to do that on your own whenever you need it outside of LB.

What about my reasoning that there will be more core mixins supplied with juggler in the future, and that they should be auto-loaded from /lib/mixins? It would immediately lead to code-duplication, if we were to split that off to lb-boot.

On the other hand, if you think this is the way to go, please go ahead and make the required changes ;-) I'd like to see it merged ...

[raymondfeng]

@fabien You are right, the CRUD methods are mixed into the model class when it's attached to a data source. The connector behind the data source provides a DataAccessObject object, which is a mixin. We can now unify these things.

For the loading, it should only apply to mixin (provider) functions. I would like to keep LDL mixin as regular models. One model can mix in other models or a mixin function can be applied.

BTW, we're trying to come up a component architecture so that NPM (or local) modules following the component layout/manifest can be automatically recognized by loopback so that models/mixins/datasources packaged in a component will be discovered by loopback.

[fabien]

@raymondfeng what's weird is that I'm using dataSourceAttached - I thought that was the right timing?

In the case of loading only mixin functions, I guess it's fine to remove the loading code here. I'd like to move on - would you like to change the code as you see fit?

Now, what I'm really looking forward to is having loopback-boot discover those npm modules, but I'd be fine with just listing the module names in config.json and go from there (default dir layout).

[raymondfeng]

The dataSourceAttached event is emitted after dataAccessConfigured, which indicates that methods from connectors have been mixed in.

[raymondfeng]

I did some clean up, see https://github.com/strongloop/loopback-datasource-juggler/tree/fabien-feature/plugins

[fabien]

@raymondfeng thanks, that looks good! So I guess lb-boot is up next? Shall I give it a shot?

[fabien]

@raymondfeng given that I'm using dataSourceAttached, why wouldn't it work as expected?

[fabien]

Check: strongloop/loopback-boot#33 - I enforced mixin name formatting, to account for files like foo-bar.js (mixin: FooBar).

[fabien]

@raymondfeng you mentioned: BTW, we're working on improving hooks so that they will be close to middleware styles and should allow chaining.

How far along is this? I'm tempted to fix this otherwise.

[raymondfeng]

I was evaluating hooks-js but it doesn't handle static methods. I forked it into https://gist.github.com/raymondfeng/51bf3f9ed2599e4ec1ad. Feel free to start from scratch or go from what I have.