Add epilogue.initialize([options.resource]) #137
Conversation
Your test wasn't being called with a context, so it was throwing an error trying to find `defaultOptions` property on `this`.
|
Fixed failed test, explaination on 41862dc commit's message. |
| @@ -78,7 +78,7 @@ describe('Resource(basic)', function() { | |||
| // TESTS | |||
| describe('construction', function() { | |||
| it('should throw an exception if called with an invalid model', function(done) { | |||
| expect(rest.resource).to.throw('please specify a valid model'); | |||
| expect(rest.resource.bind(rest)).to.throw('please specify a valid model'); | |||
There was a problem hiding this comment.
@mbroadst I had to edit this test because except was calling resource method without rest context. It doesn't change at all the purpose of this test.
|
@mbroadst do you want to note anything on this implementation? |
| storage: ':memory:', | ||
| logging: (process.env.SEQ_LOG ? console.log : false) | ||
| }), | ||
| resource: resourceOptions |
There was a problem hiding this comment.
do you think it makes a little more sense to embed this in a section called defaults? like:
epilogue.initialize({
app: {},
sequelize: new Sequelize('main', null, null, {
dialect: 'sqlite',
storage: ':memory:',
logging: (process.env.SEQ_LOG ? console.log : false)
}),
defaults: {
pagination: false
}
we already know semantically that we will be dealing with resources, so resource sounds redundant to me.
There was a problem hiding this comment.
Honestly I don't know, but I don't feel right giving non-semantic definitions on things we don't know if they will change. I've understood why you prefer so, but with defaults key I would see something more like: defaults: { resource: { /* ... */ } } (In fact I was going to implement that way but I've changed my mind because sequelize is using { define: { /* defaults for define */ } }).
My motto is always to avoid future BC by having good implementations. Even if this is only creating resources, it doesn't mean we could introduce another kind of bindings later.
But it is your decision, tell me what you think about, I will write it. :)
There was a problem hiding this comment.
I think { defaults: { resource ... is a good compromise. The existing keys are going to be global to the module (app, sequelize). Arguably, so would have been resource, as after all epilogue is chiefly interested in creating resources - however, I get your point and I think this would be a good middle ground.
There was a problem hiding this comment.
So, you are meaning it would stay as it is or do I have to change anything here?
There was a problem hiding this comment.
it would become:
epilogue.initialize({
app: {},
sequelize: new Sequelize('main', null, null, {
dialect: 'sqlite',
storage: ':memory:',
logging: (process.env.SEQ_LOG ? console.log : false)
}),
defaults: {
resource: {
pagination: false
}
}
There was a problem hiding this comment.
Regarding the fact that epilogue only creates resources, I am making up in my mind features to enhance productivity that could change that fact, but I would let that discussion to later as soon as I have an architecture well formed.
|
@cusspvz hey are you going to make that change? I think we can go ahead and merge this then |
|
Yep, I've arranged some time to work on Backend tomorrow, but I will update that on the next few hours. |
|
Any update on this? |
This PR adds the
resourceoption onepilogue.initializethat allows a definition of defaultoptionsproperties for the furtherepilogue.resourcecalls.