Prevent reInit regions on render

[JSteunou]

Now Marionette has a way to set multiple Renderer logic, it should also add a way to prevent regions to be re-initialized, because some renderer allow morphing the view by preserving actual regions content.

Either _reInitRegions shoud become public, or a new flag should be added into View.

what do you think @marionettejs/marionette-core ?

[paulfalgout]

This is nearly the same as #1263
I think not only is reiniting an issue, but so is recommending childviews be shown in onRender. Merely not clearing the region is not enough, you also cannot reshow the children which would also clear the previous children.
I would resubmit the ready event as the appropriate solution #3243

[paulfalgout]

Oh I also think this should likely be accomplished with a flag on the region much like replaceElement.
Either way I think this will require the view to "set" the el of a region rather than for the region to re-query the selector.

[JSteunou]

I had an idea for a quick solution that could be shipped into 3.5 before going all breaking for 4.0

I though we could either add a public reInitRegions which would called the private one or add a flag and put an if statement before calling the reinit.

Because changing the actual Render logic and adding a Ready event in the view lifecycle might need a major version and I think we should be able to propose this feature without breaking.

[blikblum]

Oh I also think this should likely be accomplished with a flag on the region much like replaceElement.

The flag should be in view not in region since the proposed change would affect all regions in a view.

Since the behavior is linked to the renderer an option is to pass as an parameter to setRenderer

setRenderer(myRenderer, {destructive: false})

or change the signature of renderer, instead a function an object with a render function and more options/functions. Some of the work like reinitRegions could be delegated to the renderer

I would resubmit the ready event as the appropriate solution #3243

It should work but it should not match 1:1 render event. For non destructive render would fire only in the first render

The only concern i have is that Marionette.View has already a lot of events. I would try to replace one of the events by the new one, or change the behavior of a existing

[paulfalgout]

I think a renderer wanting nondestructive regions is a single use case for nondestructive regions, but a renderer is not the only use case so it should be per region. Particularly since the current behavior is the opposite.
I am certainly interested in seeing simple solutions but I am skeptical that merely exposing the reinit function solves any real issues. I am also ok with events if they are useful and consistent.. The exchange will be unnecessarily complicated code to handle when a view renders. Honestly at this point some code POC are probably necessary to prove the point. Right now I can say in theory I support not adding an event and simply exposing a single function, but I don't know that it is realistic.

[paulfalgout]

Modifying only setRenderer however for this one use case works for me if possible and won't conflict with future nondestructive regions

[JSteunou]

like @blikblum, the use case I first though about is when setting a renderer, so all the regions of the view would be concerned. It is really tight to how the view handle its render, not on the region.

For this use case (but maybe there is another use case tight to region, and maybe it is another change) I prefer a flag on the View or as @blikblum suggested an option pass along the setRenderer. But I would suggest the option from the setRenderer set a public flag on the View, so it can also be documented and set appart the setRenderer

[paulfalgout]

I don't understand the public flag. If you do not change the rendered it would seem that the flag would merely disable the view

[JSteunou]

Yeah I might think to big and expose more things than we need to expose, but I though it could be useful for some people in order to deactivate the re-init of the regions. Maybe with some particular DOM api... I dunno

[paulfalgout]

yep I'd lean towards not exposing it until another use case is clear. I think this is actually more different than I expected originally from #1263 as really what's happening here is we're indicating that the renderer will keep the regions from being destructive because it will manage keeping the children attached, where as the other is suggesting marionette keep them nondestructive and deal with the children. As such, I would suspect that we would not want marionette handling the nondestructive regions for such a renderer either.
In any case, I do think we have possibly similar but potentially conflicting APIs if we apply them to the view, so if it is possible to solve this through the setRenderer API at least for now, that'd be good.

[JSteunou]

I still think it's quite the same than #1263 when I read the 1st post from James.

The idea is to update View.setRenderer kind of like this

  render: function render() {
    if (this._isDestroyed) {
      return this;
    }

    this.triggerMethod('before:render', this);

    // If this is not the first render call, then we need to
    // re-initialize the `el` for each region
-    if (this._isRendered) {
+    if (this._isRendered && !this._preventRegionReInit) {
      this._reInitRegions();
    }

    this._renderTemplate();
    this.bindUIElements();

    this._isRendered = true;
    this.triggerMethod('render', this);

    return this;
  },

...

}, {
  // Sets the renderer for the Marionette.View class
-  setRenderer: function setRenderer(renderer) {
+  setRenderer: function setRenderer(renderer, options) {
+    if (options.destructive === false) {
+      this.prototype._preventRegionReInit = true;
+    }
    this.prototype._renderHtml = renderer;
  },

I agree it still make sense to re-init the regions for now at each render with the default renderer behavior, so the suggestion of using a flag like deepRender from #1263 is not relevant, but I think with this change we respond to the ticket demand.

[blikblum]

I would use _destructiveRender instead of _preventRegionReInit because it could be used elsewhere like in a possible onReady implementation (see comment)

[rafde]

maybe a reRenderRegion:false on View and have

// from constructor
this.on('before:render', () => {
  const regions= this._regions;

  if(!_.isEmpty(regions)) {
    _.reduce(regions, (usedRegions, region, name) => {
      const view = region.detachView();
      if (view) {
        usedRegions[name] = view;
      }
      return usedRegions;
    }, {}); 
    this.once('render', () => {
       _.each(usedRegions, (view, name) => this.showChildView(name, view));
    });
  }
});

super rough draft and probably super naive implementation. It's also missing how dealing with replaceElement

For the cases where you only need to show child views once, maybe a onRenderOnce handler that fires once?

[paulfalgout]

@rafde I think this is beginning to address #1263 but this issue is slightly different. In this case a renderer is used that does dom diffing and the user would mark the DOM children in a way that the differ would understand not to modify or remove the node. So you can re-render the layout template and it would diff in the changes without modifying the children.. however in this case since regions are rebuilt, it breaks anyways. This is not really related to how marionette deals with children. In fact this interface will only be useful in combination with a 3rd party renderer and some 3rd party convention for keeping the children rendered in place. I think a Marionette solution for this, that works in all cases is a better feature, but I think even in the case where it exists, users of these types of renderers/conventions will want to disable it as well.

[paulfalgout]

@rafde here's a gist of a POC I made a while back related to that: https://jsfiddle.net/k60445rf/2/

It's worth noting, and I had forgotten, that in the world of non-destructive regions, the reinit goes away entirely... and to keep things "destructive" the user really only needs to keep the showChildView inside onRender

[JSteunou]

@rafde The use case is really when using diffing / patching capable renders, like morphdom or hyperhtml.

I ended up making a View like this

const HyperView = Arlequin.View.extend({
  _reInitRegions() { }
});
HyperView.setRenderer(hyperHtmlRenderer);

export default HyperView;

Which is pretty ugly you have to admit, and not to document, so we have to find a better way

[blikblum]

I support @JSteunou proposition regardless of the variable name