Skip to content
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

Add --modelsRequire option. Permits to control the models loading order #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jacargentina
Copy link

My case is: model A, depends on model B, but keystone.import seems not to know that, so when loading that way from folder ./models i got the error list B not found

With this option, i can load the models in the needed order, as done in my keystone website itself, where i load the models by doing require('./models'); and have the ./models/index.js with

require('./b.js');
require('./a.js');

@creynders
Copy link
Owner

Not saying I won't merge this, but IMO having models depend on each other is a bad code smell. That said, maybe I don't need the keystone.import at all, come to think of it... I.e. maybe it should be reqmod( opts.models, process.cwd() ); by default?

@jacargentina
Copy link
Author

An Article model depends on the User model (its author).
So, you must first load the User, and THEN the Article.

What is the bad code smell there ?

@creynders
Copy link
Owner

creynders commented Apr 30, 2016

The case with Articles and Users is best solved through a relationship, i.e. there's no need for a direct dependency there.

Edit: just realised I misunderstood your use case. But to make sure, you mean model A uses keystone.list('B'), right?

@creynders
Copy link
Owner

creynders commented Apr 30, 2016

The code smell is that the models are too tightly coupled and probably have fuzzied responsibilities and/or business logic. Possible solutions would be: merge them to one model (if applicable) or extract the dependency to a separate file.

For example if you have a model which is only valid if there are enough documents of another collection, the appropriate way would be to extract that business rule into a separate file like this:

//file: hooks/FooQux.js

const keystone = require('keystone');

keystone.list('Foo').schema.pre('save', function(done){
  keystone.list('Qux').model.count(function(n){
    if(n<=10){
        return done(new Error("Too few Qux documents!"));
    }
    done();
  })
});

Then in my keystone.js I have

keystone.import('./models');
keystone.import('./hooks');

@waltonryan
Copy link

waltonryan commented Dec 1, 2016

One of our use cases is using foreign relationships in the pre and post save methods as you described. Using the hooks methodology is a bit overkill as it adds another layer of complexity to the code.

I think the ideal solution would be to load the models and their imports irregardless of their order in the filesystem.

The solution we use now is creating folders inside /models to handle dependent vs. independent models.

Update: @creynders starting to come around on your approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants