In a LayoutView, the region's $el should be available after render

[paulovieira]

This is a simplified version of #1970

Consider this code:

var MyLV = Mn.LayoutView.extend({
    template: _.template("xyz <span class='my-region'></span>"),
    regions: {
        myRegion: ".my-region"
    }
});
var myLV = new MyLV();

myLV.myRegion.$el;  // we get [ ], as expected

myLV.render();
myLV.myRegion.$el;  // we still get [ ], but now I was expecting to get the region's element

myLV.myRegion._ensureElement();
myLV.myRegion.$el;  // now we have the region's element

That is, as part of the render process in a LayoutView, we should call _ensureElement or a similar method to obtain the region's $el immediately. Otherwise it will be available only after we call myRegion.show.

The query should be scoped to the view's element, so it's better to use myLV.$

[jamesplease]

Thanks @paulovieira. There's more to the solution than this, so I wouldn't make a PR implementing the proposed fix here. I'll do up a PR soon to see how many tests break.

[samccone]

the reason it is not around right away is because we do not do the region lookup / element cache until you show something in the reason. This is a large perf saving.

This is not a bug, and was implemented this way on purpose.

[jamesplease]

This is a large perf saving.

Is it though? Maybe in cases of lots of Regions...but I can't imagine that it is for the majority use case. If this is the reason, then I'm sure there's a better way to go about this.

This is not a bug, and was implemented this way on purpose.

Can you link to a commit / PR / issue that backs this up?

[jamesplease]

Here's my results of digging into the history of that controversial if statement in the LayoutView's render method:

The _firstRender = false; flag was added in 050bc1da0a7b because of #295, where it was pointed out that Derick's logic in the if statement was broken. This doesn't tell us much about why the if statement was added in the first place, so we go further back – past 3178318c, a small refactor – to #254.

#254 seems like a tough cookie at first. It's describing a method replacing itself when render is called, which...struck me as difficult to understand. What did he mean? If you look at the examples he provides to try to understand it, they don't provide much insight because they're broken. JSFiddle no longer works with Github source files, which is what he references. It wouldn't be too much work to get them running again, but an alternative solution is to look at the last commit before his changes. Things become a lot more clear.

Marionette's render method used to override itself every time that you called it. So it was actually impossible to bind an event to the render method, because subsequent renders would reference the old function. Yuck!

But this still doesn't explain the behavior. The rewrite got rid of an odd bit of code, but it kept the same behavior. Going back further, we run into 98bd493572f, which is where this pattern came from. It explains that it fixes #123, which is about closing a Layout's old views on subsequent renders.

So it seems like the re-initialize on the 2nd render has little to do with performance, and more about making sure that the old views are destroyed. The point of re-initialize, then, was to make sure that the regions empty themselves, and not defer the loading of Regions. tl;dr, Marionette's LayoutView used to be seriously whack.

Interesting stuff!

Anyway, the 'more complicated' solution I mentioned above is making a distinction between LayoutViews that exist at the time of creation, e.g.; el: $('.my-view'), versus ones that are constructed from a tagName, and are initially detached.

In the detached case, we want to wait until we render before we initialize the Regions. Because if we don't, there's nothing to initialize them with. In the case that the passed-in-element already exists, though, we can go ahead and instantiate the views immediately.

Is this breaking? I would say no, we're fixing a bug. But if we add this in v2.3, we're very likely to break a ton of apps, and lose a lot of community trust.

If there's anything we can take from the past 6 months of studying regions, from @jfairbank's awesome work to the slew of issues being raised about them, it's that they're really messed up right now. In the majority use-case, they work great. But the second you stray from that, you run into some pretty nasty bugs. And they're so messed up that I don't think fixing them and labeling it a 'bug fix' is going to do anyone any good.

I think our best bet is to hang back. We should rethink how Regions interface with LayoutViews, and even how they're created. In a way, we should re-imagine how this whole process works with fresh eyes, without worrying about whether we'll be breaking people's apps. or not Now that we know the problems to fix, we can come up with a better way to make this system work in v3, and then thoroughly test it.

Once we have it worked out, we'll accompany our changes with fantastic documentation so that people know exactly how the system works. That way, it'll both be easier for new users to get onboard, and hopefully also easier for older apps to be updated.

What do y'all think? @thejameskyle @jasonLaster @samccone?

[paulovieira]

Marionette's LayoutView used to be seriously whack.

What a mess this used to be :-)

Anyway, I completely agree with what you say. The ideas that have been floating around for v4 (a unified Marionette.View with regions by default - #1935) are a good answer to get rid of these and other flaws. I'd say some of these ideas should make it into v3, even if it will delay the release.

[paulfalgout]

So looking at regions.. I think the main issue here is that regions ensure their element on the show, but now that there's a getRegion interface on the View it seems like there's a much better way to do this. We should always initialize regions with an existing DOM element. And it should happen it it isn't initialized when you call getRegion. Then if you need to instantiate a region without a layout, simply pass it the DOM element.. no selector needed.

[paulfalgout]

This will be resolved here https://github.com/marionettejs/backbone.marionette/issues/3294

[paulfalgout]

Not resolved