Allow for LayoutView.prototype.render to be non-destructive

[jamesplease]

Repeatedly calling LayoutView.prototype.render will destroy all of the views within the layout's regions, and those regions themselves, and completely re-set the layout's regions.

Yikes.

That's a lot. I'm not sure if we should do all of that on a re-render. This is to discuss the possibility of changing this behavior or maybe allowing an option as a parameter of render to change the behavior.

The original discussion can be found in #877.

[algesten]

Alright. Lets kick this off. What do we mean by LayoutView.render()

• Is it just ensuring the regions are in place? (Some DOM inspection would do).
• A redraw of children? I.e. invoking render() for children?
• Is it a full "redraw" of itself? I.e. recreating the DOM for itself, invoking templates and creating new Regions? (today's behaviour)

Or something else?

[jamesplease]

I think for the sake of the community not giving up on us we'll need to keep today's behavior the default...at least for now. But we can always add options to modify that behavior or new methods to implement new functionality.

But I think instead we should start with the problems we're trying to solve. What's wrong with today's behavior? What doesn't it allow one to do easily?

[algesten]

I think of render as something quite lightweight. In my views it ensures what's on screen reflects what's in my model.

Lightweight being the key here.

What I certainly don't expect is a call to render utterly destroying my child views and obliterating child view DOM.

One can also see it as separation of concerns.

I'd expect render to handle its own DOM and delegate render to the nested views. Nothing more.

[jamesplease]

@algesten I'm interested in hearing more, but I can't think of any alternative behavior that I prefer more than what we've got. Can you elaborate more on the behavior that you think is preferable?

[pimlottc]

This was a surprise me as well quite recently. I was trying to move from a CompositeView of CompositeViews to a CompositeView of LayoutViews in order to support multiple subcollections in my inner view (see fiddle). But LayoutView does not seem to be comfortable in this role.

It seems that the philosophy of LayoutView is quite different than that of CollectionView or CompositeView, which are perfectly at home nested deep within your view hierarchy where they are subject to being repeatedly re-rendered. They look after child views for you, recreating and rerendering them as necessarily as well as providing buffering to make it efficient. In contrast, LayoutView seems to be intended to live at the top of your view hierarchy, manually created and initialized with subviews and then never (or at least sparingly) re-rendered again, as doing so requires a manual reconstruction of all the child views.

I don't know if I'm "misusing" LayoutView and should be attempting a different tack (perhaps extending CompositeView), or if LayoutView should be made more flexible to support a more dynamic use deeper within the view.

[fenduru]

I'm also running into this issue. My use case is that I have a container div that has a Title (and a couple other pieces of information), and a table of data. The "title" is just text and doesn't warrant its own view, so I want it to be part of my layout's template, and give the layout a model. I want both the title and the table to be able to re-render themselves when their models change.

I think the biggest problem is that the layout closes the contained views... which now puts a lot of strain on my controller which has to listen to the layout's render event to ensure that new views get created and shown into the region (even though it has already done that before).

I think just getting rid of the _reinitializeRegions call could go a long way, and instead, after the re-render the layout could do a $('.myRegion').replaceWith(existingRegion.$el) Perhaps it would then make sense to call render on the view -- I'm not sure

[jasonLaster]

@andrewvaslas this issue has your name on it!

I'll go on record, this issue is legit. We should make it better.

[jamesplease]

Did you mention the right person, @jasonLaster? :)

[jasonLaster]

yep - andrew discovered this bug at work.

[jamesplease]

ahh ok. cool :)

[algesten]

I think I've said this in various other issues, but I try to condense it. I think LayoutView.render() should result in:

• Ensure that the layoutview's regions are in place. First time, create the nested Region, and on consecutive calls, just ensure they are still there and that their div is intact. Separation of concern. The layoutview only cares about its own structure (and the regions), not the child views.
• On consecutive calls, delegate .render() to the shown child views. Again separation of concert, let the child views care about their own redrawing.

• Make LayoutView/Region "aware" of which child view is shown where. Example below:

  layout.reg1.show(myWidget); layout.reg2.show(myWidget);

Now reg1 still believes it holds myWidget, when in fact, it's been moved. Consecutive redraw() will cause confusion.

[samccone]

Migrated to the v3 list https://docs.google.com/document/d/1Sx1YE2SJg-NGSGsd8mELf3wpywI-UyOvCkufbm6klOw/edit#

[jamesplease]

My thoughts:

I like the default behavior of render: that it re-renders the whole subtree by default. But I understand the reasoning for wanting finer control over this. I propose a new flag you can pass to render: deepRender. By default it's true, but you can set it to false to prevent rendering the whole tree.

The technical details of this will just involve picking up the regions (or the views that they contain), then putting them back in place after the render, which @jsteunou made a good prototype of in #1722. We can throw errors if the Regions no longer exist to prevent people from doing weird things. It wouldn't be too hard to do, I think; we'll just want to make sure we adequately test it.

In addition to render, we could add this option to show as well, so that you can prevent deeply rendered layouts when showing them inside a region.

What do y'all think?

[jasonLaster]

• i like the semantics of deepRender
• what does show do?

I could be persuaded that we keep the regions around and re-insert them, but I'm not sure we want to.

It would be simpler to re-show the regions after render.

render: ->
  // render the layout
  // trigger show on the layout

There are several reasons I like this better than the alternative:

• it is simple. a cute solution at this layer will introduce lots of complexity for devs and bugs for us.
• it's consistent with showing a layout. I don't want to have different layout logic for the first render and subsequent renders. Often i have conditional logic in onShow for what to view to show, if we don't trigger show I have to figure out a way to do this manually.
• if the layout needs to be re-rendered, odds are the sub-regions do too. In this case, each subregion would also have to listen for the same event and re-render.

If we trigger show at the end of render I'd propose adding the shouldShow flag to render (render({shouldShow: true})).

[samccone]

Since a layout is both a node and contains other child nodes (regions). Render is going to blow everything away, that is just a fact.

For a non destructive render (does not re-render child nodes) we would need to investigate a very different approach. Basically region contents would be DOM fragments that would be inserted instead of being rendered into regions, but then we would hit some weird detached node issues.

Thinking about this over the past few days I don't think there is a "clean" layout based solution. Instead I see this as being something very different and perhaps even a misuse of what a layout is intended to be/do.

[jasonLaster]

One revision to the proposed api

Layout.extend({
  shouldShowOnRender: true
});

I like the class propety better because it's something that a developer would configure on the class level.

As a side-note, implementing non-destructive renders w/ the persistent region strategy should not be a difficult solution for developers either on a one-off basis with some onBeforeRender onRender logic.

[algesten]

@jmeas I don't care much for deep or shallow rendering, as I see it, that is not the crux of the problem. There is one single line in the code that is my pet peeve.

https://github.com/marionettejs/backbone.marionette/blob/master/src/marionette.layoutview.js#L38

This single row that it impossible to repeatedly call render() without utterly destroying the views shown in the regions.

[jasonLaster]

@algesten - i bet. I'm in favor of adding a flag so that we can work around it if we want to.

[samccone]

@algesten can you explain why you need to rerender the entire layout?

[JSteunou]

@samccone I dont know for him, but for me it's marionette which re-render an entire layout when this one is in a region itself. Otherwise I do not re-render entire layout manually.

[algesten]

@JSteunou nailed it. Nested layouts triggers renders on every .show()

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

I have a main app layout, and in those regions I swap in/out other layouts. My nested views get utterly destroyed.

[algesten]

The main problem is that Layout.render() is destructive. Not the recursiveness or shallowness of it.

[fenduru]

@samccone LayoutView extends from ItemView. In my LayoutView's template I should be able to put things like <%= someProperty %> and drive the LayoutView with a model. Now when someProperty changes I want that to be reflected on screen, but right now my options are:

• Create a view that does nothing but render someProperty -- too much work
• Manually do layout.$('.someProperty').text(model.get('someProperty')) -- too much work
• Re-create whatever views have been previously shown in the layout -- too much work/hard to keep code clean

While I also agree with @JSteunou 's issue where calling region.show(layoutWithChildren) will destroy the children, it is not the only issue here.

[fenduru]

This Backbone plugin uses a detach/reattach approach:

https://github.com/rotundasoftware/backbone.subviews

[jasonLaster]

Re-opening so that I can try and get a non-breaking fix in before v3.

[jamesplease]

I've been thinking about this more and more, and I am all for detaching the regions and putting them back into place after things have been re-rendered as the default behavior.

I can't wait for this to land!

[jasonLaster]

Hmm - my money is on re-calling show as things could have changed.

[jamesplease]

Can you elaborate?

[jamesplease]

@jasonLaster, do you still think you'll have time to work on a non-breaking fix for this?

[jasonLaster]

Yep. November should be good for me.

[fenduru]

Not to be a pest, but any update here @jasonLaster? Very excited for this change

[jasonLaster]

Hey @fenduru! Stop by sometime https://gitter.im/marionettejs/backbone.marionette.

This issue is definitely on my hitlist. I'm finishing up the main Inspector push. Will hopefully circle back soon though.

[samccone]

http://npmjs.org/virtual-dom

This very well could be exactly what we have been looking for :)

[jamesplease]

Minionette has a pretty clever region that supports this. Ref. I can see us implementing something similar-ish.

[megawac]

/cc @jridgewell

[jridgewell]

Minionette takes this to an extreme: I never render a subview, it's up to you to do that. Combine that with each region's placeholder view, it allows Minionette to detach and reattach the region when rendering the parent view.

It's very similar to rotundasoftware/backbone.subviews, though I don't make you insert the placeholder yourself. You can either use <%= view("nav") %> directly in the template, or provide a selector for some DOM element in the view's template. The Region docs explain the idea pretty well.

The only thing to look out for is the region's placeholder will reference a DOM element that isn't really in the parent view after calling $el.html(). You'll need call #setElement on the placeholder with a DOM element in the parent view's tree.

[ianmstew]

Leaving child view re-rendering up to the developer as an option makes sense, although I haven't had this problem myself because I write my templates/views in such a way that I never need to re-render a layout, unless the layout is trivial.

[rbu]

Marionette 3.1 now comes with a "detachChildView" function that allows you to re-attach a child view later on, which can be used to build this:

http://marionettejs.com/docs/v3.1.0/marionette.view.html#detaching-a-child-view

Thanks a bunch!

[paulfalgout]

Agreed.

There's a little bit of complexity here dealing with how to either reset or rebuild the regions.. and what to do it.. for instance you re-render the parent view, and the child view detaches.. but then the DOM element for the region renders out of the parent view.. I suppose you then destroy the child view, or do you keep it around for later? then what if you render it again and the region renders back in? what are the strategies for knowing if child views need to be re-rendered or if they're merely detached

[rafde]

moved to https://gitter.im/marionettejs/topics/topic/57fb29d884f1db0614985140/allow-for-layoutviewprototyperender-to-be-non-destructive

[paulfalgout]

As we start to look at this in 4.x I think there are 3 scenarios.

1. marionette managed destructive regions on render (what it is now)
2. marionette managed non-destructive regions (ie: marionette detaches/reattaches on render)
3. marionette non-managed regions (ie: the renderer must know to render around the regions content on render) I think we need to setup a way to allow for this to be configurable per view, but not per region.