Support loading of mixins

[fabien]

See loopbackio/loopback-datasource-juggler#201

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[offlinehacker]

+1 thanks :)

On Wed, Aug 6, 2014 at 2:49 PM, slnode [email protected] wrote:

Can one of the admins verify this patch? To accept patch and trigger a
build add comment ".ok\W+to\W+test."

—
Reply to this email directly or view it on GitHub
#33 (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-----

[fabien]

@raymondfeng I've updated this to be in line with:

loopbackio/loopback-datasource-juggler#229

[bajtos]

@fabien could you please rebase on top of current master, clean up the commit history and git push -f the result?

[bajtos]

@raymondfeng @ritch I'd like to review this patch myself, please don't merge until I approve it.

[fabien]

@bajtos note that this depends on Juggler PR loopbackio/loopback-datasource-juggler#229

[bajtos]

I am afraid loopbackio/loopback-datasource-juggler#201 contains code that does not belong to datasource juggler and should be part of loopback-boot instead. See comment1 and discussion.

@fabien Please fix that issue before proceeding with this pull request.

[fabien]

@bajtos see https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/mixins.js it's already fixed AFAIK

[bajtos]

it's already fixed AFAIK

You are right. I'll resume the review then.

[bajtos]

If I read this correctly, you have renamed options.bootDirs to options.bootSources. What is the purpose? It is also not captured in the documentation provided by index.js. Worst of all, it is a breaking change. Could you please revert?

[fabien]

OK, so I forgot to update the docs in index.js - but wouldn't you agree that it doesn't make any sense to have modelSources (a list of directories) but not bootSources? why would you have a different suffix for this option?

[bajtos]

Here is the meaning of different suffixes:

• modelSources lists places where to look for models, not all files are required
• bootDirs lists directories where all files are loaded
• bootScripts lists a files that should be added to the list of boot scripts

To stay consistent with this convention, mixins should use mixinDirs and mixinScripts.

[bajtos]

In other words, Sources suffix is used for locations where only some files are picked from, while Dir is used for locations where all files should be picked.

[bajtos]

I don't mind creating a new naming convention, as long as it clearly distinguishes those two scenarios and backward compatibility is preserved (e.g. my marking the old property name as deprecated).

[bajtos]

Moving the discussion from loopbackio/loopback-datasource-juggler#229

turns out that rejecting this PR constrains mixin names to only be referred to by their ClassNamed naming format - that doesn't sit well with me. I prefer lower-cased, dasherized for example. So unless we do this in loopback-boot, there's no way to do this.

I see two possible solutions:

• Provide the mixin name using different means than the file name. E.g. models have a name property in the JSON file that is completely unrelated to the file name.
• Implement a flag (an option) to enable/disable normalization of mixin names. I can imagine three values - none, classify, dasherize.

Thoughts?

[raymondfeng]

I think we should implement both. And the flag should also apply to model name normalizations.

[bajtos]

I think we should implement both. And the flag should also apply to model name normalizations.

In which case it may make sense to use one flag for both mixins and models. E.g. options.normalizeNames.

Implementing model name normalization is out of scope of this pull request though.

[fabien]

@bajtos @raymondfeng I'll let you guys figure that one out ;-) all I can say is that I'm still not happy with the fact that model names, and more specifically url paths (see strongloop/strong-remoting#90) are not really normalized consistently, or at least promoted as a best practice.

[bajtos]

Hi @fabien, what is the status of this pull request?

I see these major items that needs to be solved:

• a boot option for name normalization (none, classify, dasherize)
• provide the mixin name using different means than the file name. E.g. models have a name property in the JSON file that is completely unrelated to the file name.
• fix the option naming convention (Sources suffix is used for locations where only some files are picked from, while Dir is used for locations where all files should be picked.)

Is there anything else I left out? Or is there a way how to split the work into multiple smaller changes, so that this pull request can be landed sooner?

[bajtos]

@fabien ping

[bajtos]

This lines seems redundant to me, as any whitespace should have been already processed.

[fabien]

Which line exactly? and where is the whitespace processed before?

[bajtos]

Ignore this comment, I don't know why I thought whitespace was already removed.

[bajtos]

This should be options.mixinSources, not options.mixinDirs, at least according to the jsdoc comment in index.js.

Oh I see, this is more complex. The current implementation always loads all mixins, therefore mixinDirs is a more appropriate name.

However, to support isomorphic clients, we need lazy loading of mixins - only mixins that are actually used by the models should be included in the browser bundle.

To get that behaviour, we need two things:

• A way how to specify which mixins are added to the model. Is this already implemented in loopback via options.mixins property?
• A piece of code in loopback-module that will filter mixin definitions and keep only mixins used by models

My concern is that by supporting mixinDirs now, it will be difficult to add support for mixinSources later without breaking backwards compatibility or making the API confusing.

Also regarding the second way of defining mixins, via a loopback model object, this should be IMO implemented via a JSON file:

// common/mixins/address.json
{
  "properties": {
    "line1": "string",
    "line2": "string",
    "city": "string",
    "country": "string"
  }
}

@fabien I understand this may be making the scope too big. IIRC you were using a custom project layout and you needed only the executor part from this module. If that's the case, then perhaps we can scope this PR to support your use case only and implement the rest later.

Thoughts?

/cc @raymondfeng

[bajtos]

Even one more thing: you need to modify bundler too, so that mixin sources are included in the browser bundle.

[fabien]

A way how to specify which mixins are added to the model. Is this already implemented in loopback via options.mixins property?
A piece of code in loopback-module that will filter mixin definitions and keep only mixins used by models

Yes, options.mixins is already supported, as is MyModel.mixin('MixinName', { ... });

I'm not familiar with loopback-module - what is it? Same goes for the 'bundler'.

Like you said, this adds considerably to the scope of this PR, I think we should open a new one for the remaining client/isomorphic integration.

[bajtos]

I'm not familiar with loopback-module - what is it? Same goes for the 'bundler'.

Oh sorry, loopback-module should have been loopback-boot.

bundler is a part of loopback-boot, see the link in my comment above.

I'll respond in more detail next week.

[bajtos]

Stepping back a bit and looking at the big picture. IIUC, there are two kinds of mixins:

• A regular LoopBack model - mixing process copies all properties and methods to the target model.
• A function - mixing process calls the function after the model has been attached.

Mixin as a model

loopback-boot already has mechanism for defining arbitrary models, we should reuse this mechanism for defining mixins too. Benefits: developers can apply their knowledge about how to create models to create mixins too. Tools like yo loopback:model and Studio will support mixin definitions OOTB.

// common/models/timestamp-mixin.json
{
  "name": "TimeStampMixin",
  "properties": {
    "timestamp": "Date"
  }
}

// common/models/timestamp-mixin.js
module.exports = function(TimeStampMixin) {
  // setup auto-update of timestamp on save
}

To support this kind of mixins, we need to extend addAllBaseModels() and sortByInheritance() to take into account mixed-in models in addition to base models. Once that is done, model-like mixins will work both on the server and in the browserified client.

Note that this is can be implemented independently (and should be).

Mixin as a function

This is the part where we need to come up with a new convention.

Ultimately, I'd like to see a solution similar to what we have for models, where the compiler understands relations between model definitions and mixin definitions, and can build optimised instructions to load only those mixins that are actually used in the app.

Another thing to consider is the new component architecture as outlined here.

I am trying to come up with a small solution that can be incrementally extended in next PRs.

How about this:

• compiler takes two new options: mixinScripts that contains a list of source files to load, normalizeNames that defines the algorithm for converting file names into mixin/model names.
• mixin names are built from file names by the compiler, using normalizeNames
• executor is extended to define mixins per instructions from the compiler

Usage:

boot(app, {
  appRootDir: __dirname,
  mixinScripts: [ './mixins/timestamp.js' ]
});

The next step is to implement clever auto-loading of mixins from directories, as described earlier. That way the usage would become

// explicit
boot(app, {
  appRootDir: __dirname,
  mixinSources: [ './mixins' ]
});

// relying on the default mixinSources value
boot(app, __dirname)

---

To summarise the next steps as I see them now:

1. Support mixins defined as models
2. Support mixins defined as functions via mixinScripts option
3. Support mixins defined as functions via mixinSources option

---

Random notes:

• It seems to me that the setup is not handled correctly by the current mixing architecture. Imagine a TimeStampMixin as outlined above that adds a setup method to configure hooks to update the timestamp. Such setup method would override any setup method provided by the target class. This is most likely a problem of Model hooks in general as the same applies to beforeSave.
• How can a Model that is being mixed in access the mixin options provided by the app? It seems to me these options are discarded when the mixin is a Model instead of a function.

[bajtos]

@fabien what is the status of this patch? Are you still up to finish it?

[bajtos]

@raymondfeng could you please review my proposal in the comment above too?

[fabien]

@bajtos I can't look into this right now, sorry. All can add is: I don't think the use case of mixing in a Model into another Model isn't really that interesting, to me, it's just an artifact of the past, because these were the only 'mixins' originally supported.

[bajtos]

Closing in favour of #79