recent changes in backbone allow for the commonly named 'options' object to be undefined #268

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@boxxxie
Contributor
boxxxie commented Oct 4, 2012

in my app i am using lots of composite views, and they pretty much all break when going from empty to 1 item. i fixed it in this patch.

@derickbailey derickbailey commented on the diff Oct 5, 2012
src/backbone.marionette.collectionview.js
@@ -26,7 +26,15 @@ Marionette.CollectionView = Marionette.View.extend({
addChildView: function(item, collection, options){
this.closeEmptyView();
var ItemView = this.getItemView(item);
- return this.addItemView(item, ItemView, options.index);
+ var index = (function get_index(options){
@derickbailey
derickbailey Oct 5, 2012 Member

why is this wrapped in an IIFE? it would be easier to just

var index;
if (options && options.index){
  index = options.index;
} else {
  index = 0;
}
@boxxxie
boxxxie Oct 5, 2012 Contributor

i like constants

@derickbailey
derickbailey Oct 5, 2012 Member

i don't understand. what do you mean by "constants"?

the code, as is, will create a new function and execute it immediately, every time. i don't see any value in doing that. from what i can see, it just adds another layer of function definition (and memory use and performance overhead) for what can be done with a simple if statement.

is there any value in this, that i'm missing?

@boxxxie
boxxxie Oct 5, 2012 Contributor

the value is that index doesn't mutate...
other than that, no.
change it if you desire.

I don't assume what the compiler will do or not. also that code is extremely easy for a compiler to optimise as it has no state. i haven't profiled the code, i don't know how valuable that area of code is, i assume it's execution frequency will be measured in seconds, so the overhead isn't important.

personally i find that defining a variable and not assigning it to be confusing.

@derickbailey
derickbailey Oct 5, 2012 Member

the variable is assigned in all branches of the if statement that i suggested

@derickbailey
derickbailey Oct 5, 2012 Member

... just realized how all of that that came out totally douche-y. sorry about that. should have said: thanks for noticing the error! i wasn't aware of those changed in Backbone. i'll get this pulled in for the next release. :)

@boxxxie
boxxxie Oct 5, 2012 Contributor

and that is what is confusing. both solutions involve too much code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment