New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add View#dispose. #1461

Merged
merged 1 commit into from Aug 15, 2012

Conversation

Projects
None yet
@braddunbar
Collaborator

braddunbar commented Jun 27, 2012

I've taken another shot at adding a method for cleaning up view references (see #1353). I don't feel particularly strongly about the naming choice though, so please suggest others you think fit better.

The consensus seems to be that dispose should remove model listeners, remove collection listeners, and undelegate any DOM handlers with undelegateEvents. This seems like it's sufficiently flexible while still providing useful functionality.

@tkellen

This comment has been minimized.

Show comment
Hide comment
@tkellen

tkellen Jun 27, 2012

+1 for this or anything like it making its way into backbone proper.

tkellen commented Jun 27, 2012

+1 for this or anything like it making its way into backbone proper.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jun 27, 2012

Owner

Some thoughts:

  • Let's call it destroy. The symmetry with models is very desirable.
  • Do you think it needs to be paired with a declarative way of adding the model handlers in the first place, to ensure that they've been set up in a way compatible with this function?
  • DOM handlers don't need to be undelegated, do they? Let's just call this.$el.remove() instead.
Owner

jashkenas commented Jun 27, 2012

Some thoughts:

  • Let's call it destroy. The symmetry with models is very desirable.
  • Do you think it needs to be paired with a declarative way of adding the model handlers in the first place, to ensure that they've been set up in a way compatible with this function?
  • DOM handlers don't need to be undelegated, do they? Let's just call this.$el.remove() instead.
@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Jun 27, 2012

Collaborator

Let's call it destroy. The symmetry with models is very desirable.

Sounds good.

Do you think it needs to be paired with a declarative way of adding the model handlers in the first place, to ensure that they've been set up in a way compatible with this function?

I don't think so. I think it's fairly easy to make sure that listeners are compatible by passing the view as context. Further, I think it would cause confusion with events. If we are adding declarative model/collection events to views, why not routers, models, collections, etc?

DOM handlers don't need to be undelegated, do they? Let's just call this.$el.remove() instead.

I think that DOM handlers should definitely be removed, and destroy should not be conflated with remove in this regard. Not all elements have only one view attached and not all views are responsible for DOM insertion and removal. For instance, an element could be rendered by one view in a template, a view could be attached to handle click functionality, and another for rendering content. This leads to more modular code and should be encouraged IMO.

Collaborator

braddunbar commented Jun 27, 2012

Let's call it destroy. The symmetry with models is very desirable.

Sounds good.

Do you think it needs to be paired with a declarative way of adding the model handlers in the first place, to ensure that they've been set up in a way compatible with this function?

I don't think so. I think it's fairly easy to make sure that listeners are compatible by passing the view as context. Further, I think it would cause confusion with events. If we are adding declarative model/collection events to views, why not routers, models, collections, etc?

DOM handlers don't need to be undelegated, do they? Let's just call this.$el.remove() instead.

I think that DOM handlers should definitely be removed, and destroy should not be conflated with remove in this regard. Not all elements have only one view attached and not all views are responsible for DOM insertion and removal. For instance, an element could be rendered by one view in a template, a view could be attached to handle click functionality, and another for rendering content. This leads to more modular code and should be encouraged IMO.

@tbranyen

This comment has been minimized.

Show comment
Hide comment
@tbranyen

tbranyen Jun 27, 2012

Collaborator

This method seems weird to me in its current form. There are too many assumptions being made.

That not only are model and collection properties actually models and collections, but also that the events bound to them are also attached with the proper context in order for off to work correctly.

This is never called by Backbone itself so I'm not finding an extra function call worthwhile, when I could just add my own method with the stuff I need to unbind and call it from remove. I think optionally just return this so you can chain a disposal and removal like so:

myView.dispose().remove();

Its frustrating, because there are a ton of custom dispose-like functionality you may need whenever a View is removed. Shame this can't work where you just add a dispose method and it's detected in remove and calls your own cleanup code.

I'm not at all sure I agree with multiple views handling the exact same element. This seems to be an anti-pattern, not to mention I've never seen any code that does this. Do you have any references that show this done in practice? I'm just curious how this doesn't cause issues with layered events and actual render responsibility.

Collaborator

tbranyen commented Jun 27, 2012

This method seems weird to me in its current form. There are too many assumptions being made.

That not only are model and collection properties actually models and collections, but also that the events bound to them are also attached with the proper context in order for off to work correctly.

This is never called by Backbone itself so I'm not finding an extra function call worthwhile, when I could just add my own method with the stuff I need to unbind and call it from remove. I think optionally just return this so you can chain a disposal and removal like so:

myView.dispose().remove();

Its frustrating, because there are a ton of custom dispose-like functionality you may need whenever a View is removed. Shame this can't work where you just add a dispose method and it's detected in remove and calls your own cleanup code.

I'm not at all sure I agree with multiple views handling the exact same element. This seems to be an anti-pattern, not to mention I've never seen any code that does this. Do you have any references that show this done in practice? I'm just curious how this doesn't cause issues with layered events and actual render responsibility.

@tkellen

This comment has been minimized.

Show comment
Hide comment
@tkellen

tkellen Jun 27, 2012

I agree with Tim that there isn't any catch-all cleanup code that is going to work for everyone.

This is more or less how I am handling disposal now:

remove: function() {
  if(this.destroy) {
    this.destroy();
  }
  this.$el.remove();
  return this;
},

...which hits the pain point Tim mentioned: Shame this can't work where you just add a dispose method and it's detected in remove and calls your own cleanup code and creates the needed (IMO) convention for where to put that stuff.

tkellen commented Jun 27, 2012

I agree with Tim that there isn't any catch-all cleanup code that is going to work for everyone.

This is more or less how I am handling disposal now:

remove: function() {
  if(this.destroy) {
    this.destroy();
  }
  this.$el.remove();
  return this;
},

...which hits the pain point Tim mentioned: Shame this can't work where you just add a dispose method and it's detected in remove and calls your own cleanup code and creates the needed (IMO) convention for where to put that stuff.

@ianstormtaylor

This comment has been minimized.

Show comment
Hide comment
@ianstormtaylor

ianstormtaylor Jun 27, 2012

Contributor

i don't think dispose should be called from remove. there would be far more cases where you want to remove without disposing than to dispose without removing. i think jeremy's suggestion of calling remove from dispose is best, seeing as i have yet to write a dispose method that doesn't call remove.

Contributor

ianstormtaylor commented Jun 27, 2012

i don't think dispose should be called from remove. there would be far more cases where you want to remove without disposing than to dispose without removing. i think jeremy's suggestion of calling remove from dispose is best, seeing as i have yet to write a dispose method that doesn't call remove.

@paulmillr

This comment has been minimized.

Show comment
Hide comment
@paulmillr

paulmillr Jun 27, 2012

Contributor

Let's call it destroy. The symmetry with models is very desirable.

And what about models themselves? They need method that disposes mem stuff, just as views.

Contributor

paulmillr commented Jun 27, 2012

Let's call it destroy. The symmetry with models is very desirable.

And what about models themselves? They need method that disposes mem stuff, just as views.

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Jun 27, 2012

Collaborator

@tbranyen

That not only are model and collection properties actually models and collections, but also that the events bound to them are also attached with the proper context in order for off to work correctly.

I think it's safe to assume that view.model is a Model and view.collection is a Collection. Why should they be something different?

As for attaching with the proper context, it's very easy to add this as the context and this could surely be documented in a line or two.

This is never called by Backbone itself so I'm not finding an extra function call worthwhile, when I could just add my own method with the stuff I need to unbind and call it from remove. I think optionally just return this so you can chain a disposal and removal like so:

I would be ok with a noop implementation, and I think it may be the best option due to the variance in usage.

Its frustrating, because there are a ton of custom dispose-like functionality you may need whenever a View is removed. Shame this can't work where you just add a dispose method and it's detected in remove and calls your own cleanup code.

I don't think we should conflate removal with disposal. One does not imply the other.

I'm not at all sure I agree with multiple views handling the exact same element. This seems to be an anti-pattern, not to mention I've never seen any code that does this. Do you have any references that show this done in practice? I'm just curious how this doesn't cause issues with layered events and actual render responsibility.

I often use multiple views with the same element to promote code reuse. For instance, one view handles setting content (a translation, a model attribute, or a collection property) and another handles click/hover/keyboard functionality (destroy a model, change an attribute, save a model, or fetch a collection). I shouldn't have to write all the permutations of those things when combining them is much easier and more testable.

Collaborator

braddunbar commented Jun 27, 2012

@tbranyen

That not only are model and collection properties actually models and collections, but also that the events bound to them are also attached with the proper context in order for off to work correctly.

I think it's safe to assume that view.model is a Model and view.collection is a Collection. Why should they be something different?

As for attaching with the proper context, it's very easy to add this as the context and this could surely be documented in a line or two.

This is never called by Backbone itself so I'm not finding an extra function call worthwhile, when I could just add my own method with the stuff I need to unbind and call it from remove. I think optionally just return this so you can chain a disposal and removal like so:

I would be ok with a noop implementation, and I think it may be the best option due to the variance in usage.

Its frustrating, because there are a ton of custom dispose-like functionality you may need whenever a View is removed. Shame this can't work where you just add a dispose method and it's detected in remove and calls your own cleanup code.

I don't think we should conflate removal with disposal. One does not imply the other.

I'm not at all sure I agree with multiple views handling the exact same element. This seems to be an anti-pattern, not to mention I've never seen any code that does this. Do you have any references that show this done in practice? I'm just curious how this doesn't cause issues with layered events and actual render responsibility.

I often use multiple views with the same element to promote code reuse. For instance, one view handles setting content (a translation, a model attribute, or a collection property) and another handles click/hover/keyboard functionality (destroy a model, change an attribute, save a model, or fetch a collection). I shouldn't have to write all the permutations of those things when combining them is much easier and more testable.

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Jun 27, 2012

Collaborator

@paulmillr

And what about models themselves? They need method that disposes mem stuff, just as views.

Is there any reason not to use model.on('destroy', ...) for this?

Collaborator

braddunbar commented Jun 27, 2012

@paulmillr

And what about models themselves? They need method that disposes mem stuff, just as views.

Is there any reason not to use model.on('destroy', ...) for this?

@paulmillr

This comment has been minimized.

Show comment
Hide comment
@paulmillr

paulmillr Jun 27, 2012

Contributor

@braddunbar destroy does HTTP DELETE to server. It's completely different. Also, dispose is a stuff that can be overrided by child models to dispose more things. Simple event binding will work shitty here.

Say, you have a controllers that dispatch routes and do this sort of stuff

  show: (params) ->
    user = new User({login: params.login})
    @model = new Repo({user, name: params.repoName})
    @view = new RepoPageView({@model})
    @model.fetch()

When user changes URL to something different, this stuff (@model, @view) need to be disposed, all references should be deleted. But it shouldn't be deleted on server. Chaplin does this correctly:

class Model extends Backbone.Model
  ...

  # Disposal
  # --------

  disposed: false

  dispose: ->
    return if @disposed

    # Fire an event to notify associated collections and views
    @trigger 'dispose', this

    # Unbind all global event handlers
    @unsubscribeAllEvents()

    # Remove all event handlers on this module
    @off()

    # If the model is a Deferred, reject it
    # This does nothing if it was resolved before
    @reject?()

    # Remove the collection reference, internal attribute hashes
    # and event handlers
    properties = [
      'collection',
      'attributes', 'changed'
      '_escapedAttributes', '_previousAttributes',
      '_silent', '_pending',
      '_callbacks'
    ]
    delete this[prop] for prop in properties

    # Finished
    @disposed = true

    # You’re frozen when your heart’s not open
    Object.freeze? this
Contributor

paulmillr commented Jun 27, 2012

@braddunbar destroy does HTTP DELETE to server. It's completely different. Also, dispose is a stuff that can be overrided by child models to dispose more things. Simple event binding will work shitty here.

Say, you have a controllers that dispatch routes and do this sort of stuff

  show: (params) ->
    user = new User({login: params.login})
    @model = new Repo({user, name: params.repoName})
    @view = new RepoPageView({@model})
    @model.fetch()

When user changes URL to something different, this stuff (@model, @view) need to be disposed, all references should be deleted. But it shouldn't be deleted on server. Chaplin does this correctly:

class Model extends Backbone.Model
  ...

  # Disposal
  # --------

  disposed: false

  dispose: ->
    return if @disposed

    # Fire an event to notify associated collections and views
    @trigger 'dispose', this

    # Unbind all global event handlers
    @unsubscribeAllEvents()

    # Remove all event handlers on this module
    @off()

    # If the model is a Deferred, reject it
    # This does nothing if it was resolved before
    @reject?()

    # Remove the collection reference, internal attribute hashes
    # and event handlers
    properties = [
      'collection',
      'attributes', 'changed'
      '_escapedAttributes', '_previousAttributes',
      '_silent', '_pending',
      '_callbacks'
    ]
    delete this[prop] for prop in properties

    # Finished
    @disposed = true

    # You’re frozen when your heart’s not open
    Object.freeze? this
@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Jun 27, 2012

Collaborator

@braddunbar destroy does HTTP DELETE to server. It's completely different.

Right. I didn't mean call model.destroy(). I meant listen for a "destroy" event and do cleanup at that point (possibly triggered manually, rather than as a result of model.destroy()).

I do, however, understand your point which, I think, is that view.destroy would be used to clean up memory references while model.destroy submits an HTTP DELETE - a very different functionality.

Collaborator

braddunbar commented Jun 27, 2012

@braddunbar destroy does HTTP DELETE to server. It's completely different.

Right. I didn't mean call model.destroy(). I meant listen for a "destroy" event and do cleanup at that point (possibly triggered manually, rather than as a result of model.destroy()).

I do, however, understand your point which, I think, is that view.destroy would be used to clean up memory references while model.destroy submits an HTTP DELETE - a very different functionality.

@chadhietala

This comment has been minimized.

Show comment
Hide comment
@chadhietala

chadhietala Jun 28, 2012

@braddunbar I think what @paulmillr is trying to say is that backbone should release models from memory that are not needed at a specific route. Waiting for a destroy event still has the potential of models being held in memory, so they should be released as soon as they are not needed.

chadhietala commented Jun 28, 2012

@braddunbar I think what @paulmillr is trying to say is that backbone should release models from memory that are not needed at a specific route. Waiting for a destroy event still has the potential of models being held in memory, so they should be released as soon as they are not needed.

@paulmillr

This comment has been minimized.

Show comment
Hide comment
@paulmillr

paulmillr Jun 28, 2012

Contributor

Actually no, I don't use backbone router at all. Just a convention for disposing models from memory would be super cool because I don't understand why views should have it and models shouldn't.

Contributor

paulmillr commented Jun 28, 2012

Actually no, I don't use backbone router at all. Just a convention for disposing models from memory would be super cool because I don't understand why views should have it and models shouldn't.

@eastridge

This comment has been minimized.

Show comment
Hide comment
@eastridge

eastridge Jun 29, 2012

Contributor

Just a quick note, I was looking at this pull request and saw Thorax incorrectly cited:

#1353

We have a method named freeze which unbinds the events on a view related to a given model or collection, but have also standardized on using a destroy method to do all of our cleanup. Just my two cents for this discussion: destroy seems like a fine name for this method and I agree that it shouldn't by default do anything in particular.

Contributor

eastridge commented Jun 29, 2012

Just a quick note, I was looking at this pull request and saw Thorax incorrectly cited:

#1353

We have a method named freeze which unbinds the events on a view related to a given model or collection, but have also standardized on using a destroy method to do all of our cleanup. Just my two cents for this discussion: destroy seems like a fine name for this method and I agree that it shouldn't by default do anything in particular.

@anthonyshort

This comment has been minimized.

Show comment
Hide comment
@anthonyshort

anthonyshort Jun 30, 2012

I think I agree with @paulmillr here. Calling the method destroy is confusing when compared to the functionality of Model#destroy. A common method name for across all components would make more sense as you can assume methods with that name have the same responsibility.

anthonyshort commented Jun 30, 2012

I think I agree with @paulmillr here. Calling the method destroy is confusing when compared to the functionality of Model#destroy. A common method name for across all components would make more sense as you can assume methods with that name have the same responsibility.

@eastridge

This comment has been minimized.

Show comment
Hide comment
@eastridge

eastridge Jun 30, 2012

Contributor

One doesn't assume that when initialize is called on a model it's going to do the work of initializing a view. Why would destroy be any different?

Contributor

eastridge commented Jun 30, 2012

One doesn't assume that when initialize is called on a model it's going to do the work of initializing a view. Why would destroy be any different?

@anthonyshort

This comment has been minimized.

Show comment
Hide comment
@anthonyshort

anthonyshort Jul 1, 2012

I see your point, but models often need the ability to clean up after themselves as well. As do collections. So having a common method for that would be preferred, I think.

anthonyshort commented Jul 1, 2012

I see your point, but models often need the ability to clean up after themselves as well. As do collections. So having a common method for that would be preferred, I think.

@politician

This comment has been minimized.

Show comment
Hide comment
@politician

politician Jul 11, 2012

View#tearDown from onsi/coccyx handles garbage collecting a View pretty well. It's worth a look to compare APIs.

politician commented Jul 11, 2012

View#tearDown from onsi/coccyx handles garbage collecting a View pretty well. It's worth a look to compare APIs.

@tobias74

This comment has been minimized.

Show comment
Hide comment
@tobias74

tobias74 Jul 26, 2012

maybe there could also be a machanism to manage child-views:

a parent view which has spawn multiple child-views has to call destroy on its child-views when destroy is called on itself.

This could be done in the base-clase of Backbone.View. The parent-view could then call a method like this.addChildView(childView) to add the child-view to a list of managed child-views.

Does this make any sense?

tobias74 commented Jul 26, 2012

maybe there could also be a machanism to manage child-views:

a parent view which has spawn multiple child-views has to call destroy on its child-views when destroy is called on itself.

This could be done in the base-clase of Backbone.View. The parent-view could then call a method like this.addChildView(childView) to add the child-view to a list of managed child-views.

Does this make any sense?

@eastridge

This comment has been minimized.

Show comment
Hide comment
@eastridge

eastridge Jul 26, 2012

Contributor

@tobias74 definitely out of scope for core backbone.

Contributor

eastridge commented Jul 26, 2012

@tobias74 definitely out of scope for core backbone.

jashkenas added a commit that referenced this pull request Aug 15, 2012

@jashkenas jashkenas merged commit 3ae1af6 into jashkenas:master Aug 15, 2012

@veesahni

This comment has been minimized.

Show comment
Hide comment
@veesahni

veesahni Aug 30, 2012

@jashkenas would it be worthwhile to also include this.off() in dispose() to ensure any subscriptions to custom view events are also cleaned up?

veesahni commented Aug 30, 2012

@jashkenas would it be worthwhile to also include this.off() in dispose() to ensure any subscriptions to custom view events are also cleaned up?

@timkurvers

This comment has been minimized.

Show comment
Hide comment
@timkurvers

timkurvers Aug 31, 2012

Strongly siding with @ianstormtaylor here, dispose should remove the actual DOM-element. remove however, should most definitely not dispose of the view. The current implementation will break re-attaching views at a later date. One would have to explicitly re-initialize these views, which is highly undesirable.

+1 on calling this.off() as @veesahni remarked.

In our current disposal-implementation we also this.trigger('dispose', this) prior to executing the actual disposal routine to allow any listeners to react appropriately. This could potentially conflict with any custom dispose-event users may be emitting. Regardless, this could be useful.

timkurvers commented Aug 31, 2012

Strongly siding with @ianstormtaylor here, dispose should remove the actual DOM-element. remove however, should most definitely not dispose of the view. The current implementation will break re-attaching views at a later date. One would have to explicitly re-initialize these views, which is highly undesirable.

+1 on calling this.off() as @veesahni remarked.

In our current disposal-implementation we also this.trigger('dispose', this) prior to executing the actual disposal routine to allow any listeners to react appropriately. This could potentially conflict with any custom dispose-event users may be emitting. Regardless, this could be useful.

@philfreo

This comment has been minimized.

Show comment
Hide comment
@philfreo

philfreo Aug 31, 2012

Contributor

Like @ianstormtaylor and @timkurvers I also was surprised to see remove calling dispose instead of the other way around.

While doing it this way prevents people in some common cases from having to change their code (if they were naively forgetting to cleanup themselves), it doesn't make sense to me that simply removing a view from the DOM (even temporarily) would stop the view object (which may intentionally be kept around) from still listening on a Model or Collection.

Think about it like this: View.remove() by default is supposed to be a no-op (since by default a view isn't in the DOM). However, calls to this.model.on() usually happen in the view's initialize, so if you have remove() call dispose() then remove is almost never a no-op anymore!

With the idea that a view should be able to be kept around and re-inserted into the DOM, I'd propose:

    // Dispose of this view by removing it from the DOM and cleaning up 
    // any references to it, to prevent latent effects and memory leaks
    dispose: function() {
      if (this.model) this.model.off(null, null, this);
      if (this.collection) this.collection.off(null, null, this);
      this.remove();
      this.off();
      return this;
    },

    // Remove this view from the DOM. Note that the view isn't present in the
    // DOM by default, so calling this method may be a no-op.
    remove: function() {
      this.undelegateEvents();
      this.$el.remove();
      return this;
    },

I left undelegateEvents inside of remove because it seems like that might be the one thing you always want to do when removing from the DOM. But not 100% sure there.

With this change, I also think this.off() makes sense, once you've clarified that you're explicitly disposing of the view rather than simply removing it from the DOM.

Contributor

philfreo commented Aug 31, 2012

Like @ianstormtaylor and @timkurvers I also was surprised to see remove calling dispose instead of the other way around.

While doing it this way prevents people in some common cases from having to change their code (if they were naively forgetting to cleanup themselves), it doesn't make sense to me that simply removing a view from the DOM (even temporarily) would stop the view object (which may intentionally be kept around) from still listening on a Model or Collection.

Think about it like this: View.remove() by default is supposed to be a no-op (since by default a view isn't in the DOM). However, calls to this.model.on() usually happen in the view's initialize, so if you have remove() call dispose() then remove is almost never a no-op anymore!

With the idea that a view should be able to be kept around and re-inserted into the DOM, I'd propose:

    // Dispose of this view by removing it from the DOM and cleaning up 
    // any references to it, to prevent latent effects and memory leaks
    dispose: function() {
      if (this.model) this.model.off(null, null, this);
      if (this.collection) this.collection.off(null, null, this);
      this.remove();
      this.off();
      return this;
    },

    // Remove this view from the DOM. Note that the view isn't present in the
    // DOM by default, so calling this method may be a no-op.
    remove: function() {
      this.undelegateEvents();
      this.$el.remove();
      return this;
    },

I left undelegateEvents inside of remove because it seems like that might be the one thing you always want to do when removing from the DOM. But not 100% sure there.

With this change, I also think this.off() makes sense, once you've clarified that you're explicitly disposing of the view rather than simply removing it from the DOM.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Aug 31, 2012

Owner

@veesahni -- I don't think so. If this view is removed, and the other views that are listening to it are removed (or even if they haven't been yet) ... it should still be able to be GC'd. The listener objects exist in the removed view, not externally.

@timkurvers, @philfreo -- We're trying to help with the (perhaps) common case of folks wanting to tear down views that are observing models that have a much longer lifespan than the views do. Removing a view from the DOM to be inserted later is a pretty strange pattern -- it's just as easy, and probably conceptually simpler just to hide it. That said, if all you want to do is remove a view from the DOM, leaving its event listeners intact, just do:

view.$el.remove();
Owner

jashkenas commented Aug 31, 2012

@veesahni -- I don't think so. If this view is removed, and the other views that are listening to it are removed (or even if they haven't been yet) ... it should still be able to be GC'd. The listener objects exist in the removed view, not externally.

@timkurvers, @philfreo -- We're trying to help with the (perhaps) common case of folks wanting to tear down views that are observing models that have a much longer lifespan than the views do. Removing a view from the DOM to be inserted later is a pretty strange pattern -- it's just as easy, and probably conceptually simpler just to hide it. That said, if all you want to do is remove a view from the DOM, leaving its event listeners intact, just do:

view.$el.remove();
@brettjonesdev

This comment has been minimized.

Show comment
Hide comment
@brettjonesdev

brettjonesdev Dec 11, 2012

Absolutely! This will greatly aid clean unbinding and help prevent memory leaks.

brettjonesdev commented Dec 11, 2012

Absolutely! This will greatly aid clean unbinding and help prevent memory leaks.

@mkuklis

This comment has been minimized.

Show comment
Hide comment
@mkuklis

mkuklis Dec 11, 2012

+1 for listenTo and stopListening I do like the subscribe and unsubscribe naming convention proposed by @andriijas

mkuklis commented Dec 11, 2012

+1 for listenTo and stopListening I do like the subscribe and unsubscribe naming convention proposed by @andriijas

@2color

This comment has been minimized.

Show comment
Hide comment
@2color

2color Dec 11, 2012

+1 to the listenTo and the stopListening. Makes it a whole lot easier cleaning up complex views.

2color commented Dec 11, 2012

+1 to the listenTo and the stopListening. Makes it a whole lot easier cleaning up complex views.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Dec 11, 2012

Owner

@kevinrobinson -- It's not baked in to the view class. It's in Backbone.Events.

@amasad -- I thought about that too, but I'm keen on the simplicity of saying ... if you mix in Backbone.Events, your object is now able to participate in the universe of emitting and listening to events. Backbone already naturally works like this, and I can see how it could simplify other areas, like collections tracking their models, as well.

Owner

jashkenas commented Dec 11, 2012

@kevinrobinson -- It's not baked in to the view class. It's in Backbone.Events.

@amasad -- I thought about that too, but I'm keen on the simplicity of saying ... if you mix in Backbone.Events, your object is now able to participate in the universe of emitting and listening to events. Backbone already naturally works like this, and I can see how it could simplify other areas, like collections tracking their models, as well.

@ianstormtaylor

This comment has been minimized.

Show comment
Hide comment
@ianstormtaylor

ianstormtaylor Dec 11, 2012

Contributor

+1 — I'd also vote for subscribe and unsubscribe because I much prefer single-word method names.

Contributor

ianstormtaylor commented Dec 11, 2012

+1 — I'd also vote for subscribe and unsubscribe because I much prefer single-word method names.

@johnnyoshika

This comment has been minimized.

Show comment
Hide comment
@johnnyoshika

johnnyoshika Dec 11, 2012

+1 from me as well on listenTo and stopListening.

johnnyoshika commented Dec 11, 2012

+1 from me as well on listenTo and stopListening.

@caseywebdev

This comment has been minimized.

Show comment
Hide comment
@caseywebdev

caseywebdev Dec 11, 2012

Collaborator

@ianstormtaylor so subscribeTo and unsubscribeFrom? 😉

Collaborator

caseywebdev commented Dec 11, 2012

@ianstormtaylor so subscribeTo and unsubscribeFrom? 😉

@Integralist

This comment has been minimized.

Show comment
Hide comment
@Integralist

Integralist Dec 11, 2012

+1 for renaming to subscribe/unsubscribe as the current names are FUGLY

Integralist commented Dec 11, 2012

+1 for renaming to subscribe/unsubscribe as the current names are FUGLY

@caseywebdev

This comment has been minimized.

Show comment
Hide comment
@caseywebdev

caseywebdev Dec 11, 2012

Collaborator

@github should add a poll feature.

Collaborator

caseywebdev commented Dec 11, 2012

@github should add a poll feature.

@KidkArolis

This comment has been minimized.

Show comment
Hide comment
@KidkArolis

KidkArolis Dec 11, 2012

+1 for this new proposal (of extending Backbone.Event with listenTo and stopListening). A couple of comments.

  • now that remove() is cleaning up event listeners, should it also call this.off() to remove all event listeners attached via the view.on. I guess it's not really needed, because you should always prefer the new view.listenTo
  • alternative names for these methods are also bindTo/unbindFrom or observe/stopObserving, but I'm starting to like listenTo/stopListening it seems to read well
  • I feel I might continue using Marionette's (@derickbailey's) EventBinder, because it can unify the event binding API of Backbone and jQuery, e.g. you can bind to dom events using view.bindTo($("window"), "resize", this.render), etc. I suppose that could be achieved with a plugin extending the native implementation as it's hard to cater for all the use cases..

KidkArolis commented Dec 11, 2012

+1 for this new proposal (of extending Backbone.Event with listenTo and stopListening). A couple of comments.

  • now that remove() is cleaning up event listeners, should it also call this.off() to remove all event listeners attached via the view.on. I guess it's not really needed, because you should always prefer the new view.listenTo
  • alternative names for these methods are also bindTo/unbindFrom or observe/stopObserving, but I'm starting to like listenTo/stopListening it seems to read well
  • I feel I might continue using Marionette's (@derickbailey's) EventBinder, because it can unify the event binding API of Backbone and jQuery, e.g. you can bind to dom events using view.bindTo($("window"), "resize", this.render), etc. I suppose that could be achieved with a plugin extending the native implementation as it's hard to cater for all the use cases..
@TheCloudlessSky

This comment has been minimized.

Show comment
Hide comment
@TheCloudlessSky

TheCloudlessSky Dec 11, 2012

@philfreo Yup, that' makes sense. It really is a matter of semantics right now.

TheCloudlessSky commented Dec 11, 2012

@philfreo Yup, that' makes sense. It really is a matter of semantics right now.

@johnnyoshika

This comment has been minimized.

Show comment
Hide comment
@johnnyoshika

johnnyoshika Dec 11, 2012

@KidkArolis, you shouldn't have to call this.off() on the view if you're disposing it, because if nothing else references it, it will get garbage collected. It doesn't hurt to do it, but it shouldn't be necessary.

I've named my methods bindTo / unbindFrom. It made more sense in the past as it offered symmetry with Backbone's previous method names (bind / unbind). I'm fine with whatever names Backbone chooses though. I'm sure I'll get used to it.

johnnyoshika commented Dec 11, 2012

@KidkArolis, you shouldn't have to call this.off() on the view if you're disposing it, because if nothing else references it, it will get garbage collected. It doesn't hurt to do it, but it shouldn't be necessary.

I've named my methods bindTo / unbindFrom. It made more sense in the past as it offered symmetry with Backbone's previous method names (bind / unbind). I'm fine with whatever names Backbone chooses though. I'm sure I'll get used to it.

@paulmillr

This comment has been minimized.

Show comment
Hide comment
@paulmillr

paulmillr Dec 11, 2012

Contributor

this is good shit! thanks, Jeremy. although i’d used subscribeEvent and unsubscribe(all)Event(s) naming.

this.subscribeEvent(model, 'change', this.render);
this.unsubscribeAllEvents(model);
this.unsubscribeAllEvents();

subscribe will work shitty, consider subscribe event in User model, which creates user subscription to magazine or subscribe event in View, which does the same.

Contributor

paulmillr commented Dec 11, 2012

this is good shit! thanks, Jeremy. although i’d used subscribeEvent and unsubscribe(all)Event(s) naming.

this.subscribeEvent(model, 'change', this.render);
this.unsubscribeAllEvents(model);
this.unsubscribeAllEvents();

subscribe will work shitty, consider subscribe event in User model, which creates user subscription to magazine or subscribe event in View, which does the same.

@arbales

This comment has been minimized.

Show comment
Hide comment
@arbales

arbales Dec 12, 2012

Really glad to see this in backbone. I also typically prefer shorter method names, but also like the semantics of listenTo. My vote would be for #observe and #stopObserving for brevity's sake. Failing that, I prefer #listen over #subscribe.

👏

Sent from my thumbs.

On Dec 12, 2012, at 4:43 AM, Paul Miller notifications@github.com wrote:

this is good shit! thanks, Jeremy. although i’d used subscribeEvent and unsubscribeEvent naming.


Reply to this email directly or view it on GitHub.

arbales commented Dec 12, 2012

Really glad to see this in backbone. I also typically prefer shorter method names, but also like the semantics of listenTo. My vote would be for #observe and #stopObserving for brevity's sake. Failing that, I prefer #listen over #subscribe.

👏

Sent from my thumbs.

On Dec 12, 2012, at 4:43 AM, Paul Miller notifications@github.com wrote:

this is good shit! thanks, Jeremy. although i’d used subscribeEvent and unsubscribeEvent naming.


Reply to this email directly or view it on GitHub.

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Dec 12, 2012

observe is also good name indeed. listen is something i do with radio. :)

andriijas commented Dec 12, 2012

observe is also good name indeed. listen is something i do with radio. :)

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Dec 12, 2012

Collaborator

observe and ignore, perhaps?

Collaborator

braddunbar commented Dec 12, 2012

observe and ignore, perhaps?

@arbales

This comment has been minimized.

Show comment
Hide comment
@arbales

arbales Dec 12, 2012

I value the tenseness and semantics of #observebut I prefer#stopObservingvs#ignore` for the sake of clarity and explicitness.

Sent from my thumbs.

On Dec 12, 2012, at 6:16 PM, brad dunbar notifications@github.com wrote:

observe and ignore, perhaps?


Reply to this email directly or view it on GitHub.

arbales commented Dec 12, 2012

I value the tenseness and semantics of #observebut I prefer#stopObservingvs#ignore` for the sake of clarity and explicitness.

Sent from my thumbs.

On Dec 12, 2012, at 6:16 PM, brad dunbar notifications@github.com wrote:

observe and ignore, perhaps?


Reply to this email directly or view it on GitHub.

@philfreo

This comment has been minimized.

Show comment
Hide comment
@philfreo

philfreo Dec 12, 2012

Contributor

I value the tenseness and semantics of #observebut I prefer#stopObservingvs#ignore` for the sake of clarity and explicitness.

+1

Contributor

philfreo commented Dec 12, 2012

I value the tenseness and semantics of #observebut I prefer#stopObservingvs#ignore` for the sake of clarity and explicitness.

+1

@dgbeck

This comment has been minimized.

Show comment
Hide comment
@dgbeck

dgbeck Dec 12, 2012

Very cool idea.

In terms of naming, another option is to use the same on and off functions and check the parameter types to see what "style" of event binding the caller wants to use. The idea is to always prefer the new "listenTo" style syntax and ideally the old "on" sytax would be depreciated / discouraged? If that is the case seems like "on" and "off" are the best words, its just that they are already taken!

dgbeck commented Dec 12, 2012

Very cool idea.

In terms of naming, another option is to use the same on and off functions and check the parameter types to see what "style" of event binding the caller wants to use. The idea is to always prefer the new "listenTo" style syntax and ideally the old "on" sytax would be depreciated / discouraged? If that is the case seems like "on" and "off" are the best words, its just that they are already taken!

@wjohnald

This comment has been minimized.

Show comment
Hide comment
@wjohnald

wjohnald Dec 12, 2012

+1 for "observe"

wjohnald commented Dec 12, 2012

+1 for "observe"

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Dec 12, 2012

Owner

While observe/stopObserving is very close, I think that I'm going to leave the method names as listenTo/stopListening -- mostly for clarity.

I'm a bit worried about the possibility for confusion between:

view.unbind()
view.stopObserving()

... where of course the former is removing all events bound to the view, and the latter removes all of the events that the view is listening to on other objects. I think that phrasing it this way makes the difference clearer:

view.on('open', action);

view.listenTo(model, 'change', doSomething);

view.off('open');

view.stopListening();
Owner

jashkenas commented Dec 12, 2012

While observe/stopObserving is very close, I think that I'm going to leave the method names as listenTo/stopListening -- mostly for clarity.

I'm a bit worried about the possibility for confusion between:

view.unbind()
view.stopObserving()

... where of course the former is removing all events bound to the view, and the latter removes all of the events that the view is listening to on other objects. I think that phrasing it this way makes the difference clearer:

view.on('open', action);

view.listenTo(model, 'change', doSomething);

view.off('open');

view.stopListening();
@arbales

This comment has been minimized.

Show comment
Hide comment
@arbales

arbales Dec 13, 2012

Beyond brevity, I think that #observe fits more with the inversion of control that's happening here. And, I don't feel that #stopListening is any clearer than #stopObserving with regards to confusion with standard binding. If anything, an observer stands out more than a listener, especially given that the Events API uses the latter term already.

That along with the tenseness/attractiveness of #observe makes better in my view.

Finally, white bike sheds stay cooler in Indian heat, which traveling this December is making me more sensitive to.

Sent from my thumbs.

On Dec 13, 2012, at 3:55 AM, Jeremy Ashkenas notifications@github.com wrote:

While observe/stopObserving is very close, I think that I'm going to leave the method names as listenTo/stopListening -- mostly for clarity.

I'm a bit worried about the possibility for confusion between:

view.unbind()
view.stopObserving()
... where of course the former is removing all events bound to the view, and the latter removes all of the events that the view is listening to on other objects. I think that phrasing it this way makes the difference clearer:

view.on('open', action);

view.listenTo(model, 'change', doSomething);

view.off('open');

view.stopListening();

Reply to this email directly or view it on GitHub.

arbales commented Dec 13, 2012

Beyond brevity, I think that #observe fits more with the inversion of control that's happening here. And, I don't feel that #stopListening is any clearer than #stopObserving with regards to confusion with standard binding. If anything, an observer stands out more than a listener, especially given that the Events API uses the latter term already.

That along with the tenseness/attractiveness of #observe makes better in my view.

Finally, white bike sheds stay cooler in Indian heat, which traveling this December is making me more sensitive to.

Sent from my thumbs.

On Dec 13, 2012, at 3:55 AM, Jeremy Ashkenas notifications@github.com wrote:

While observe/stopObserving is very close, I think that I'm going to leave the method names as listenTo/stopListening -- mostly for clarity.

I'm a bit worried about the possibility for confusion between:

view.unbind()
view.stopObserving()
... where of course the former is removing all events bound to the view, and the latter removes all of the events that the view is listening to on other objects. I think that phrasing it this way makes the difference clearer:

view.on('open', action);

view.listenTo(model, 'change', doSomething);

view.off('open');

view.stopListening();

Reply to this email directly or view it on GitHub.

@andriijas andriijas referenced this pull request Dec 13, 2012

Closed

Event's map proposal #287

@meleyal

This comment has been minimized.

Show comment
Hide comment
@meleyal

meleyal Dec 14, 2012

The way we do unbinding currently is to have every view listen to a global event (turbolinks' page:change in our case) that triggers view#uninstall (i.e. dispose). Encapsulated this in Backbone.PluginView.

What I've found useful is the idea of a namespace for the view that can be used when binding, unbinding and triggering custom events (see jQuery namespaced events, #740).

meleyal commented Dec 14, 2012

The way we do unbinding currently is to have every view listen to a global event (turbolinks' page:change in our case) that triggers view#uninstall (i.e. dispose). Encapsulated this in Backbone.PluginView.

What I've found useful is the idea of a namespace for the view that can be used when binding, unbinding and triggering custom events (see jQuery namespaced events, #740).

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