Backbone Model defaults system is not subclass safe

[BradNeuberg]

Backbone provides a standard 'defaults' object literal that you can add to a Model to establish defaults. However, if you are subclassing models then this is not safe.

For example, imagine if you have:

var Car = Backbone.Model.extend({
defaults: {
property1: 'foobar'
}
});

var Toyota = Car.extend({
defaults: {
property2: 'hello'
}
});

When Toyota is made property2 will be present, but not property1.

Backbone should instead have a getDefaults() method that provides an object literal. Subclasses can over-ride this (and call their super-classes getDefaults() to chain defaults together) so that when Backbone mixes its defaults in it can aggregate them all together, with something like:

Backbone.Model = function(attributes, options) {
attributes || (attributes = {});
attributes = _.extend({}, this.getDefaults(), attributes);

Where the default getDefaults() implementation would just be an empty {}.

[marcalj]

The same problem is present for the events object in views.

[shesek]

The defaults property can be used as a function, https://github.com/documentcloud/backbone/blob/master/backbone.js#L136.

Backbone.View's events don't support that, and maybe support should be added, but for now you could call delegateEvents() manually with the events as an argument from initialize() (and make sure to call the super's initialize() too)

[jashkenas]

Yep, if you'd like to use super etc, define your member properties as functions instead...

[machineghost]

Rather than making everyone use functions for their defaults, can I propose a different solution (and hopefully get this ticket re-opened)?

Backbone uses its special:
var extend = function (protoProps, classProps) {
var child = inherits(this, protoProps, classProps);
child.extend = this.extend;
return child;
};
to handle sub-classing. Why not simply add to that method so that it extends defaults rather than replacing them? Something like:

var extend = function (protoProps, classProps) {
var child = inherits(this, protoProps, classProps);
if (typeof this.defaults != "function: && typeof protoProps.defaults != "function") {
child.defaults = _.extend({}, this.defaults, protoProps.defaults);
}
child.extend = this.extend;
return child;
};

That's not production-ready code, but hopefully you get the idea. If this were implemented, everyone could sub-class to their heart's content AND get to use objects for their defaults ...

var A = Backbone.Model.extend({defaults:{foo:"bar"}});
var B = A.extend({defaults:{bar:"baz"}});
assert new B().get("foo") == "bar";
assert new B().get("bar") == "baz";

[braddunbar]

@machineghost Model.extend is the same function that is used for Collection, View, and Router. Altering it to allow this functionality would also change the other classes. Also, using __super__ is much more flexible. For instance, what if I don't want to extend the parent's defaults but rather replace them? What if I just want two specific properties and to ignore the rest?

[machineghost]

Well if you want to do something fancy, clearly functions are the way to go. But it seems to me that 80% of the time (complete BS figure) when someone wants default inheritance they want simple inheritance, ie. "keep the values that come from my parent (or my parent's parent, or my ...) unless I define a different value". I'm assuming this because it's how both OOP and prototypal inheritance works. If you delete all values from a parent class because a sub-class dares to define a single value (even if it's a completely different value from any set by the parent), you're doing something very unfamiliar to anyone who is familiar with inheritance.

As for "this functionality would also change other classes" ... we're talking about five lines here right? Five lines that I imagine don't change very often. Making a new modelExtend copy of the existing function would add neither significant weight nor would it be a maintainability problem (if you change extend once a year, you have to change the same line twice, once a year; and does extend even change once a year?).

So, I hear what you're saying; you've got this great simple extension mechanism, and it will be less simple if you change it in the way I propose. But given that it's not getting problematically less simple, and given that it would give users the kind of inheritance they expect from an inheritance framework, isn't it worth it?

[machineghost]

Just to try and show why this matters, here's defaults inheritance done using the two different approaches:

// My proposed form
var A = Backbone.Model.extend({
defaults:{foo:1}
});
var B = A.extend({
defaults:{bar:1}
});

// Current form
var A = Backbone.Model.extend({
defaults: function() {
return {foo:1};
}
});
var B = A.extend({
defaults: function (){
return _.extend(this.constructor.super.defaults(), {{bar:1}
}
});

Now imagine you're a new Backbone user:
A) you probably don't even know that defaults can be a function
B) even if you do know, you probably don't know about this.constructor or this.constructor.super
C) even if you know about that, you still have to put it all together with extend (something that's easy for a user familiar with extending stuff, but that's not everyone)
D) you very likely don't even know you need to do any of this. There's no warning sign on the documentation for defaults that says "hey, this stuff doesn't inherit!", so until the lack of inheritance bites you you likely just keep using the object form of defaults, expecting it to work

If the only issue is keeping the special extend function "DRY" (don't repeat yourself), and not forking it in to two functions ... is that really worth causing all of the above confusion?

[braddunbar]

when someone wants default inheritance they want simple inheritance, ie. "keep the values that come from my parent (or my parent's parent, or my ...) unless I define a different value".

This is exactly what's happening. The value of defaults from the parent's prototype is kept unless it's redefined.

[machineghost]

The value of defaults from the parent's prototype is kept unless it's redefined.

So, that's totally one way to think of it; defaults are all one property. From an implementation standpoint, that's exactly the right way to think about it, because the defaults are literally a single JS property.

But, that ignores how Backbone actually works/is used: you don't do fooModel.get(), you do fooModel.get("bar"). In other words, attributes isn't just "one property"; Backbone encourages you to think (and rightly so) that each attribute is a property of the model. foo.get("bar") != foo.get("baz"), even though foo.attributes == foo.attributes.

So, I'd argue that because Backbone doesn't work directly with .attributes, and because it does have get/set/a bunch of other methods that treat each attribute as a property, the defaults of the attributes should be thought of the same way: not as a single defaults property, but as a fooDefault, a barDefault, etc.

And when you think of it that way, it makes absolutely no sense why setting a fooDefault would have any effect whatsoever on your barDefault.

[tauren]

I might have a related problem. Once I changed my defaults to a function, my problem went away. I haven't dug into the problem further to determine the root cause.

I defined defaults as an object, not a function in a base class:

defaults: {
  messages: {}
}

I then extended that base class and instantiated several instances of the extended model and added properties to the messages attribute:

var model1 = new Extended(),
  model2 = new Extended();

model1.get('messages').doIt = 'message 1';
console.log(model2.get('messages').doIt);  // message 1

The problem is that both model1.attributes.messages and model2.attributes.messages reference the same object. This was not at all expected. Making defaults a function fixed it.

I'm guessing that a deep copy of the defaults object is not being done.

[braddunbar]

@tauren You're right, no copying of defaults is being done so any non-primitive value you set will be used for all instances of that type. Using a function creates a new defaults object on every call and sidesteps the issue.

[tauren]

@braddunbar Yep, and in fact the docs do mention this. I need to re-read the docs, as the last time I went through them was quite a while ago.

Remember that in JavaScript, objects are passed by reference, so if you include an object as a default value, it will be shared among all instances.

[gigafied]

This is the biggest pitfall I see from developers switching from a classical inheritance language into prototypical inheritance (JavaScript).

In the case of Backbone, because you actually call it defaults` it really should be getting cloned every time you extend a Model, View, etc.

This is very simple to do, in the inherits method. Something like:

child.prototype.defaults = _.clone(child.prototype.defaults || {});

[machineghost]

The shared reference issue is totally a valid one, but why stop with cloning when Backbone can just as easily provide actual inheritance on defaults? Remember, clone is nothing but a wrapper around extend, so instead of the base clone extend:

// This is essentially the same as _.clone(child.prototype.defaults) child.prototype.defaults = _.extend({}, child.prototype.defaults);

Backbone could do:

child.prototype.defaults = _.extend({}, parent.prototype.defaults, child.prototype.defaults);

and then a the default for Foo on class A wouldn't be erased by setting a default on Bar on class B (that subclasses A).

[machineghost]

Braddunbar, that issue was closed with a documentation-only change. I'm trying to advocate for an actual fix (which would also be a fix for the events issue because they both use the same behavior of replacing rather than extending).

[braddunbar]

@machineghost Backbone is already providing a prototypal inheritance scheme. Model.defaults and View.events are just like any other property and can be extended/changed/merged in the same fashion. I think that changing the paradigm for one or two properties will only cause more confusion around the issue. And this is not to mention the serious backward compatibility problems this would cause.

[machineghost]

Backbone is already providing a prototypal inheritance scheme.

Not all prototypal inheritance systems are equal ;-) And that's the thing: this isn't about prototypal inheritance vs. classical, it's about two different views of how to implement prototypal inheritance in Backbone.

What it basically comes down to is that you can look at defaults and events as individual properties, but if you're going to do that then why not look at attributes as an individual property too? When someone does set("foo", "bar"), behind the scenes let's do attributes = {"foo": "bar").

Now that would (obviously) be silly, because "attributes" isn't a property, it's a collection of properties. Similarly, events and defaults aren't just individual properties, and I really think continuing to pretend that they are is only going to cause more problems and make the framework weaker overall. As soon as you start treating them the same way you treat attributes, suddenly this issue goes away. And likewise, the issue with objects in defaults goes away. (Plus, I'd think most use-cases for function-based defaults would go away too).

Implementation-wise none of this is hard; just use _.extend and call it a day. As for backwards-compatability:
A) Backbone isn't even 1.0 yet; now is the perfect time to fix design decisions made in the past which have ill effects, but couldn't have been foreseen
B) There are plenty of strategies for handling this sort of thing. Just one example ...

1. make a Model and Collection class that implements the old behavior; let's say SimpleInheritanceModel and SimpleInheritanceCollection
2. make Model and Collection use extend for defaults and events (and if I'm missing any similar "properties that are really collections of properties", fix them too)
3. add a method to Backbone called "useSimpleInheritance". If called, this method would set Backbone.Model = SimpleInheritanceModel and Backbone.Collection = SimpleInheritanceCollection.

There you go; now every new user gets proper inheritance on events and defaults that works the same as the rest of Backbone (ie. attributes) does, and doesn't require warnings like "Remember that in JavaScript, objects are passed by reference, so if you include an object as a default value, it will be shared among all instances." (warnings which are easily missed and confuse new users).

Existing Backbone users will likely welcome these changes (but don't take my word for it: you could always make a poll asking people if they want simple inheritance or extend-based inheritance). If some don't, or if they just can't afford to fix issues that might crop up they just have to add a single line (Backbone.useSimpleInheritance()), which they can add at the same time they increment the version number in the file name (because none of this affects them until they upgrade). Heck, maybe some people will even continue using this option for performance reasons in the future ... but most will probably think extend-based inheritance is so natural, they'll quickly forget defaults/events ever did anything differently.

So basically, everyone wins with this approach.

[marcalj]

@machineghost 👍

[moos]

Just got bit by this!! Seems unexpected behavior from conventional OO inheritance model, where subclasses inherit not only methods but properties (attributes) of parent class.

I'd like to see this feature added -- or a non-clunky alternative provided -- perhaps a 'defaults2.0' :), '_defaults', 'inheritableDefaults' , etc. if backward compatibility is the issue.

[akre54]

@moos the simple answer is if you want inheritance, you should build it yourself. see: https://github.com/documentcloud/backbone/issues/search?q=events+extend

You can also take the approach that Chaplin takes and collect instances up the prototype chain yourself: chaplinjs/chaplin#295 and utils.coffee#L60-67

[ORESoftware]

I don't think you should build inheritance in JS yourself. Just avoid it. Use composition.