Fix #1339 - Add Backbone.View#destroy.

[braddunbar]

As stated in #1339, destroy is a fairly strong convention as this point. Should Backbone reinforce this convention or leave it to the user? If so, I'm of the opinion that it should be left blank so that the user can fill it in. Even the most minimal default implementation would have drawbacks and make assumptions about the structure of the user's code.

[braddunbar]

If any implementation is to be provided, I think it should be very minimal. For instance:

if (this.model) this.model.off(null, null, this);
if (this.collection) this.collection.off(null, null, this);

[molily]

I’m a strong supporter of adding these things to Backbone, at least as a convention. I have to admit I’m biased since I released a Backbone-based library which has a focus on object disposal and memory management. ;)

In Chaplin, we’re using the name dispose on all core classes since destroy is already taken on Model (where it has a totally different meaning). I’d love to see conventions for disposing models and collections in Backbone as well, but I guess you consider this to be out of scope. (Well, I thought this for View as well, good to see some progress here.)

As an overview, Thorax is calling it freeze, Marionette is calling it close. While Thorax and Marionette are using an event unbinding abstraction, Chaplin is already using the model./collection.off(null, null, this) solution since Backbone.Events supported it.

How about paving the cowpaths here?

[tbranyen]

I've called it cleanup in LayoutManager since that's what you're actually doing. I'd recommend calling it that instead.

2 cents

[aterris]

I too think that it makes sense to solidify this convention. I would lean towards a complete no-op to keep this as light as possible, but could see the case for the simple implementation posted above

[tgriesser]

+1 on adding it with a base implementation rather than a no-op as it's very minimal and would likely be the most common use case (unlike render, which varies widely)

[wookiehangover]

@braddunbar any documentation to go along with this? IMO these stubbed in methods are only worthwhile if there are clear usage guidelines in the docs..

[braddunbar]

@wookiehangover Good point, fleshing out docs will probably be informative as to including or not including this. I'll do a draft.

[webbower]

I just started using Backbone and came to a similar conclusion. I'm glad I'm not the only one. I went with teardown. I add the following to my project to shim on global functionality:

Backbone.View.prototype.remove = function() {
    this.teardown().$el.remove();
    return this;
};

Backbone.View.prototype.teardown = function() {
    return this;
};

Then I would just override teardown on each View that needed additional cleanup.

[braddunbar]

I've added some initial documentation for the destroy stub, along with an example usage. I think that it's best left unimplemented for the time being since this.model.off(null, null, this) is still a fairly new convention.

I think that adding some convention for destroying views has been unanimously approved so I'm going to merge this as is. However, please continue to comment or open issues regarding naming, implementation, or documentation.

[onsi]

Along similar lines, I wrote Coccyx to provide teardown-able view hierarchies. The intent is to provide a way to plug up leaky view hierarchies: just registerSubview()s as you make them and call tearDown() on a root view to bring the entire hierarchy down.

Coccyx also monkey-patches .on and .off (I know, I know, awful... "but it works, and I wrote tests around it"). This allows all contextualized event bindings (not just bindings to view.model and view.collection) to get ripped out when view.tearDown() is called.

[webbower]

I think this.undelegateEvents() should be added to the destroy stub. As of right now, Backbone Views don't clean themselves up internally (I understand why you're hesitant to bake in this.model.off()). Granted, it's the developer's responsibility to clean up after themselves, but I think that if you have a declarative why to add functionality automatically on init, the default teardown method should mirror that setup.

[tgriesser]

@webbower jQuery cleans up all events bound with delegateEvents() when $el.remove() is called, so it probably wouldn't be necessary to add that to the destroy stub.

[braddunbar]

@tgriesser That's true, but not all views call remove, especially if they're not the only view attached to the element. I think undelegateEvents would be a good start to an implementation (if one is to be provided).

[jashkenas]

Whoops -- I didn't realize this got merged in. Can we roll it back out of master, and discuss further?

If we're going to standardize a "destroy" method for views, I think it might as well not be a no-op. Some views won't need it ... and for the views that do need it, it might as well work out of the box.

[paulmillr]

@jashkenas some ideas for implementations of destroy / dispose:

https://github.com/chaplinjs/chaplin/blob/master/src/chaplin/models/model.coffee#L78
https://github.com/chaplinjs/chaplin/blob/master/src/chaplin/views/view.coffee#L364

[braddunbar]

Sure thing. Reverted in d8477f4.

[nervetattoo]

+1 for this as a non no-op, a default implementation doing undelegateEvents, model.off and collection.off would suit our needs quite well.

We've standardized on freeze and unfreeze in our code, and I must say I prefer the explicitness of this.freeze().remove() for cleaning up, especially as we use freeze to disable views without pulling them from the DOM so we can unfreeze them a little later.

[kof]

Just wanted to create the same issue ... I am also using .destroy on views, collections and models. My destroys do the same thing supposed here earlier - unbinding all events the view/collection/model has listened to + .remove on views.

1. It is a VERY logical consequence of .destroy call
2. on views .destroy should call .remove after unbinding events.
3. Most of backbone users don't even think about unbinding events.

I have started to use destroy a long time ago, after the application started to get slower and slower. I started to investigate why. The reason was mostly views, which have been removed from the dom, but still listened to the events from collections/models and has processed/rendered stuff in stealth mode.

So it is a real issue with real use cases. Everybody should use destroy and be warned to use it in the documentation.

+100 for .destroy on everything.

[kof]

Think about socket.io which can be used in collection/models .. it will listen on events and views will react on them the whole time. Switching views by removing the previous one will create an endless amount of event handlers rendering stuff behind the scenes.

[kof]

jQuery is unbinding events from dom elements being removed via jquery. Backbone should follow the similar logic.

[kof]

Something like this + current Model#destroy logic for model.

Backbone.View.prototype.destroy =
Backbone.Collection.prototype.destroy = 
Backbone.Model.prototype.destroy = function() {
    if (this.collection) this.collection.off(null, null, this);
    if (this.model) this.model.off(null, null, this);

    // Remove element if in view.
    if (this.$el) this.$el.remove();

    // Unbind own events.
    this.off();
};

[akre54]

@kof I'd imagine the majority of the memory leaks you're seeing could be fixed by using listenTo and stopListening instead of on/off, and following a pattern where on will only be used for objects that live longer than the calling object for easy cleanup. For what it's worth, Backbone calls stopListening in View#remove to automatically unbind your events.

[kof]

@akre54 thanks for the info about View#remove, I also completely forgot about listenTo, because I am still using 0.9.2 version.

I just read some issues discussing why listenTo was introduced ... but I still not liking it.
I don't know any usecase where a view should remain listening to events after it is destroyed ...
I don't like .listenTo vs. .on ....
I will implement my #destroy methods and not going to use listenTo
Also .listenTo is only for views, what if a model is listening to a collection and has been destroyed/remove ...?

Its not nicely looking and inconsistent.

[caseywebdev]

@kof listenTo is a method in Events, and is therefore extended onto Model, Collection, Router, View, and even Backbone itself along with all of the other event methods. It's a great way to listen to other objects while not having to keep track of those other objects yourself. Instead, when you're done listening, simply call stopListening().

[kof]

ok right, listenTo seems to be really the most clear way ...

I have played with the idea to define more default namespaces like 'models, collections, views' etc. and unbind all events from them on remove/destroy, but it isn't very intuitiv if instantiating something new inside of a view f.e..

Also introducing an array where you need to put everything you listen to in order to stop listening on remove/destroy could be an option.

[kof]

off topic ... but

1. shouldn't Collection#remove call Model#stopListening, like View#remove?
2. shouldn't both places also call this.off() to remove all listener on the own instance?

[mponizil]

Hey @kof,

1. Model#stopListening would remove all of that model's listeners. The model still exists after Collection#remove, it's just no longer part of the collection.

2. Again, removing a model from a collection does not mean anything has been discarded- the only cleanup needed is the "all" event, which is taken care of with Collection#_removeReference:

   https://github.com/documentcloud/backbone/blob/master/backbone.js#L654

   As for turning off external listeners in View#remove, I'm not sure that's entirely necessary or intuitively correct behavior. If you have other modules listening for events from that view, those modules should probably take care of cleaning up those listeners on their own- behaving otherwise would break down some separation of concerns.

[akre54]

@kof the general rule of thumb (to paraphrase @mehcode from another discussion) is if the lifetime of your target is shorter than this, use target.on(...), else use this.listenTo(target, ...).

[kof]

1. you are right, this should happen in Model#destroy
2. we could use .off(null, null, this) - so only cleanup events used inside of the current instance

@akre54 @mponizil do you know any use cases, where after a view/model has been destroyed, somebody who was listening on it should still receive any events?
I think if it is really destroyed, not just removed from the DOM or from the collection there is no.

The problem is now - we have no "destroy".

So actually View#remove does too much now, If I just want to remove a view from the dom and insert it somewere else, I will need to add listener again. Same for $el.remove() of jquery.

It seems to me if we want to clearly separate "remove/detach" from complete "destroy" we need this "destroy" thing.

[kof]

@mponizil also there should be 'remove' event on the view if other listeners should stop listening to the view on remove.

[kof]

Also there is no real conflict with current Model#destroy implementation. Current Model#destroy just should accept an options.sync and only if its true, send the 'delete' event. Otherwise just do the cleanups.