backbone.marionette
backbone.marionette copied to clipboard
marionettejs
Allow for LayoutView.prototype.render to be non-destructive
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.
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?
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?
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.
@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?
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.
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
@andrewvaslas this issue has your name on it!
I'll go on record, this issue is legit. We should make it better.
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.
Migrated to the v3 list https://docs.google.com/document/d/1Sx1YE2SJg-NGSGsd8mELf3wpywI-UyOvCkufbm6klOw/edit#
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?
- 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
onShowfor what to view to show, if we don't triggershowI 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})).
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.
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.
@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.
@algesten - i bet. I'm in favor of adding a flag so that we can work around it if we want to.
@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.
@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.
The main problem is that Layout.render() is destructive. Not the recursiveness or shallowness of it.
@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.
This Backbone plugin uses a detach/reattach approach:
https://github.com/rotundasoftware/backbone.subviews
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, do you still think you'll have time to work on a non-breaking fix for this?