Region.open uses jQuery.empty() which destroys event handlers

[algesten]

When I move around an ItemView between different regions, the item is eventually broken so that no event handlers work anymore. This is despite overriding .close (which otherwise would result in stopListening).

The problem is Region.open, which uses jQuery.empty()

https://github.com/marionettejs/backbone.marionette/blob/master/src/marionette.region.js#L158

From the jQuery doc:

"To avoid memory leaks, jQuery removes other constructs such as data and event handlers from the child elements before removing the elements themselves."

[samccone]

@algesten can you provide a code example.

Based on what you have said tho, once you close a view instance you can not just reuse it. You have to call delegateEvents() on it to rebind the events.

[algesten]

Notice I say I override .close to not close my itemview – this is not the problem. But I'll make a test example later today. Thxs.

[algesten]

Here's an example. See the line where it says "uncomment" to show the problem. http://jsfiddle.net/mP6Qg/

Similar example but in coffeescript:

class MyItemView extends Marionette.ItemView
    events:
        'click': 'onClick'
    # These items are not closed
    close: ->
    onClick: =>
        console.log('click')

class MyLayout extends Marionette.Layout
    regions:
        theregion: '.theregion'

iv1 = new MyItemView()
iv1.render()

iv2 = new MyItemView()
iv2.render()

layout = new MyLayout()
layout.render()
$('body').appendChild layout.el

layout.theregion.show(iv1)
layout.theregion.show(iv2)

# at this point iv1 is in a bad state
# because the second .show() caused
# Region.open() do an jQuery.clear()
# with iv1 attached.

The jQuery.empty() effectively causes the equivalent of a stopListening, but the view is not in a closed state.

[jamesplease]

This is the intended behavior; just override it.

[algesten]

Lol.

[jamesplease]

@algesten, I hope you didn't mistake my terseness for rudeness. That wasn't my intention at all. I was just trying to communicate that Marionette methods are meant to be overrode, but obviously you know that given the fact that you've changed close. It's clear that I was reviewing issues far too late last night, as I think I'm understanding the problem more clearly now.

Do you think that close should handle the closing down of a region, including removing the event listeners, and open should just populate it with new content, calling close if there's something there?

So maybe something like:

open: function() {
  if (this.currentView) { this.close() };
  this.$el.append(view.el);
}

close: function() {
  // ...the usual
  this.$el.empty();
}

@algesten, is that more along the lines of what you're thinking? This makes a lot of sense to me. @cobbweb / @samccone?

[algesten]

Thanks. I was a bit miffed. But happy now :)

I think my main problem is that we have explicit API methods undelegate and stopListening so I don't like the situation where jQuery has "side effects" that double up on our API. If our intended behaviour is to stop listening for DOM events we should make that an explicit call to our own API.

As you identify above, the problem is made worse by that it is open that destroys the event handlers of the previous view.

I think my preferred solution would be to.

1. As you suggest, move the DOM emptying to the Region.close method.
2. Not use jQuery.empty(), but instead do this.el.innerHTML = ''.

This way we rely on our own ItemView.close() to stop listening and the DOM clearing has no side effects.

[samccone]

Be aware of not using jQuery.empty(), you are basically opening yourself to memory leaks.

[Anachron]

@samccone yes that is what he was suggesting. Why are you using jQuery.empty() instead of this.el.innerHTML = ''?

[algesten]

But separation of concerns. Surely it's not up to the Region to manage the potential memory leakage of an ItemView?

The correct solution would be for ItemView.close() to ensure there are no lingering DOM handlers.

[jamesplease]

I'm receptive to these arguments. A region's element has no events attached to it when you're using it in an out-of-the-box configuration of Marionette. The view classes might, but those events are attached to the elements within the view, and as @algesten points out by managing it in the region we seem to be mixing up responsibilities.

Unless there's something I'm missing, I think using innerHTML here might make the most sense.

[jamesplease]

This discussion and #970 seem to be transitioning a region to more standalone class of thing, which I think I like. Instead of tightly coupling it to whatever is inside of it it instead becomes just a container. #970 raises an interesting problem of keeping track of where things are, though.

[samccone]

We have talked about using innerHTML but decided against it due to the memory problems it opens us to.

https://github.com/marionettejs/backbone.marionette/issues/842

[algesten]

@samccone The view is (correctly) concerned about its own state and is correctly using jQuery.html() in render to avoid potential memory leak caused by itself.

The region could be defensive and use jQuery.empty() (on close) but should at the very least check view.isClosed (or view.el.parentNode) after doing view.close() before destroying the state of the attached view.

Which triggers another thought. I could completely avoid the problem by overriding my close as this:

class MyItemView extends Backbone.Marionette
    close: => @$el.remove()

[jamesplease]

I'm thinking that it's pretty clear that we should remove $.empty() from the open function. A separation of responsibilities would involve close shutting things down, and show/open setting them up.

A shortcut that some people use is to just call show on a region that already has an item view instantiated. Folks who do this might be worried that this change would prevent that from working. I don't think that it'd cause any problem on account of open() already calling close when there's a different view already in the region.

So the least that we could do is move this to close. But is it even necessary? As @algesten points out, the region shouldn't be responsible for removing event listeners for the view. I agree with that; the view should handle itself when it gets shut down. And it looks like it does do that when it calls this.$el.remove() in its remove function.

If this is all true we should just be able to remove the $.empty function altogether (you know, in v2) with no side effects.

[algesten]

I agree it should go. Problem is that this.$el.append(view.el); would not replace an existing view in the region (just add it after). view.close() does solve that since it removes itself from the DOM, which leaves the border case for those dirty view-reusers like myself :)

For people like me, separation of concerns would mean that the region is responsible for ensuring it is only showing one view at a time (that is the purpose of region). As such it shouldn't depend on view.close() to do its job.

Which leaves me at having a non-destructive DOM emptying such as .innerHTML = '' in Region.close() would make most sense. After all, for regular non-reusers, it's only doubling up for view.close() doing $.remove(), so I don't agree it's a memory leak concern here.

[jamesplease]

@algesten, agreed about .innerHTML as the most logical choice. Added bonuses are the big performance boost over append, and that it would work better for someone who wants to reuse their views for performance reasons.

It seems a bit odd to me that append was chosen in the first place to be used right after an $.empty, actually. But maybe there's a good reason behind it. Looks like we're doing it because of this post. Some interesting points are made, but performance doesn't seem to be a concern in the article. :+1: for innerHTML.

[jamesplease]

WHAT TO TAKE FROM THIS

region.open should only do OPEN things. Wtf up wit this, yo?

Even further, regions shouldn't be managing the DOM events of its children, anyway! It should never call empty, right? If your code is susceptible to memory leaks once this code is removed, then your code is probably poorly written. Views' close methods should handle their own listeners.

[algesten]

@jmeas I wholeheartedly agree :)

[Anachron]

@jmeas :+1:

[jamesplease]

@algesten good news – the core just met and we all agree that this will be in v2. I'll be working on a PR sometime over the next week...I'd love for you to review it once it's ready.

[algesten]

Great! Of course I'll help reviewing it. Just holler.

[ahumphreys87]

ok so I had an attempt at this, its not as easy as it sounds, mainly due to the fact that this.el is not a DOM element, its a selector: this test case https://github.com/ahumphreys87/backbone.marionette/commit/cd122c4ef4988419e5fe54ca9e9e3f1130fbf124 shows why this wont work, without a bigger change (unless I'm missing something)

[algesten]

The original code empties $el (not el). What if we do the same but with innerHTML? How about something like

this.$el.each(function(){this.innerHTML=""});

Not that I expect $el to hold more than one DOM element, but it's jQuery, so perhaps it's prudent.

[jamiebuilds]

Fixed in #946

[jamesplease]

:tada: