Don't ignore default options in the view constructor when it's a function. #819
Conversation
| @@ -13,7 +13,7 @@ Marionette.View = Backbone.View.extend({ | |||
| // this is a backfill since backbone removed the assignment | |||
| // of this.options | |||
| // at some point however this may be removed | |||
| this.options = _.extend({}, this.options, options); | |||
| this.options = _.extend({}, _.result(this, 'options'), options); | |||
There was a problem hiding this comment.
@seansfkelley if we are going to add this which I am not opposed to let's also add a conditional _.isFunction on the options passed into the constructor
|
That shouldn't be necessary; the interaction between extend and result should handle all three cases of nonexistent, literal, and function (added a test to make sure of this). I can add a test for the nonexistent case, if you'd like. |
_.extend({}, function(){return {bar: 1}}) === {}All i am saying is if we are going to allow functional evaluation, might as well do this then this.options = _.extend({}, _.result(this, 'options'), _.isFunction(options) ? options.call(this) : options ); |
|
Right, I just realized I'd misread after I stepped away from the computer! We could do that as well, however, I was merely trying to recreate the old Backbone behavior, which notably did not treat specially a function as argument. Has there been a use case for this? Personally I have not seen one, and as I use Backbone 1.0 right now I would err on the side of simply mimicking it. |
|
Fair comment, I think that allowing for the functional evaluation of the options param could be handy in some cases. |
|
+1 |
|
@samccone Done and good catch. |
|
👍 |
|
1 little thing before a merge, the format of this commit message is not according to the ideal way. see http://git-scm.com/book/ch5-2.html Once that is fixed we can merge |
Fixes an issue where Marionette would not respect a function that returned an object as the `options` field of a View.
Don't ignore default options in the view constructor when it's a function.
Fixed #818