Fix #1339 - Add Backbone.View#destroy. #1353

Merged
merged 2 commits into from Jun 3, 2012

Conversation

Projects
None yet
@braddunbar
Collaborator

braddunbar commented May 28, 2012

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

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar May 28, 2012

Collaborator

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);
Collaborator

braddunbar commented May 28, 2012

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

This comment has been minimized.

Show comment
Hide comment
@molily

molily May 28, 2012

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?

molily commented May 28, 2012

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

This comment has been minimized.

Show comment
Hide comment
@tbranyen

tbranyen May 29, 2012

Collaborator

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

2 cents

Collaborator

tbranyen commented May 29, 2012

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

This comment has been minimized.

Show comment
Hide comment
@aterris

aterris May 29, 2012

Contributor

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

Contributor

aterris commented May 29, 2012

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

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser May 30, 2012

Collaborator

+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)

Collaborator

tgriesser commented May 30, 2012

+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

This comment has been minimized.

Show comment
Hide comment
@wookiehangover

wookiehangover May 30, 2012

Collaborator

@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..

Collaborator

wookiehangover commented May 30, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar May 30, 2012

Collaborator

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

Collaborator

braddunbar commented May 30, 2012

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

@webbower

This comment has been minimized.

Show comment
Hide comment
@webbower

webbower May 30, 2012

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.

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

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Jun 3, 2012

Collaborator

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.

Collaborator

braddunbar commented Jun 3, 2012

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.

braddunbar added a commit that referenced this pull request Jun 3, 2012

Merge pull request #1353 from braddunbar/destroy
Fix #1339 - Add Backbone.View#destroy.

@braddunbar braddunbar merged commit 7054ca4 into jashkenas:master Jun 3, 2012

@onsi

This comment has been minimized.

Show comment
Hide comment
@onsi

onsi Jun 24, 2012

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.

onsi commented Jun 24, 2012

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

This comment has been minimized.

Show comment
Hide comment
@webbower

webbower Jun 24, 2012

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.

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

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser Jun 24, 2012

Collaborator

@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.

Collaborator

tgriesser commented Jun 24, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Jun 25, 2012

Collaborator

@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).

Collaborator

braddunbar commented Jun 25, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jun 25, 2012

Owner

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.

Owner

jashkenas commented Jun 25, 2012

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.

braddunbar added a commit that referenced this pull request Jun 25, 2012

Revert #1353 for further discussion.
This reverts commit 7054ca4, reversing
changes made to 7828d6d.
@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Jun 25, 2012

Collaborator

Sure thing. Reverted in d8477f4.

Collaborator

braddunbar commented Jun 25, 2012

Sure thing. Reverted in d8477f4.

@nervetattoo

This comment has been minimized.

Show comment
Hide comment
@nervetattoo

nervetattoo Jun 27, 2012

Contributor

+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.

Contributor

nervetattoo commented Jun 27, 2012

+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.

@braddunbar braddunbar referenced this pull request Jun 27, 2012

Merged

Add View#dispose. #1461

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 2, 2013

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 commented Apr 2, 2013

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

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 2, 2013

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 commented Apr 2, 2013

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

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 2, 2013

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

kof commented Apr 2, 2013

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

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 2, 2013

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();
};

kof commented Apr 2, 2013

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

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Apr 2, 2013

Collaborator

@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.

Collaborator

akre54 commented Apr 2, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 3, 2013

@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.

kof commented Apr 3, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@caseywebdev

caseywebdev Apr 3, 2013

Collaborator

@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().

Collaborator

caseywebdev commented Apr 3, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 3, 2013

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 commented Apr 3, 2013

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

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 3, 2013

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?

kof commented Apr 3, 2013

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

This comment has been minimized.

Show comment
Hide comment
@mponizil

mponizil Apr 3, 2013

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.

mponizil commented Apr 3, 2013

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

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Apr 3, 2013

Collaborator

@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, ...).

Collaborator

akre54 commented Apr 3, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 3, 2013

  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 commented Apr 3, 2013

  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

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 3, 2013

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

kof commented Apr 3, 2013

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

@kof

This comment has been minimized.

Show comment
Hide comment
@kof

kof Apr 3, 2013

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.

kof commented Apr 3, 2013

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.

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