onBeforeAttach and onAttach for CollectionView child views (continued)

[ianmstew]

Continuation of #2441, fixes #2209.

New feature of this PR:

• Solved how to pass down Region#show()'s triggerBeforeAttach and triggerAttach flags.

Needs the following wrapup, probably 1-2 hours' work, will do in next day or so:

• Make final call on view.once('render' to trigger before:attach within _addChildView and addEmptyView. See onBeforeAttach and onAttach for CollectionView child views #2441 (comment). @jasonLaster I wouldn't mind talking this one through more.
• Additional specs (will bump coverage to 100%)
  • When collectionView is shown in a region with triggerBeforeShow == false
  • When collectionView is shown in a region with triggerShow == false
• Implement @paulfalgout's suggestion for triggering before:attach
• Final squash

[ianmstew]

Looks like Travis choked :/

[paulfalgout]

restarting the choked travis :-)

[jasonLaster]

nice - @ianmstew... mind summarizing the discussion for once as a comment so everyone can weigh in

[ianmstew]

Okay everyone, please weigh in on this.

Region#show()

      if (attachedRegion && triggerBeforeAttach) {
        displayedViews = this._displayedViews(view);
        this._triggerAttach(displayedViews, 'before:');
      }

      this.attachHtml(view);
      this.currentView = view;

      if (attachedRegion && triggerAttach) {
        displayedViews = this._displayedViews(view);
        this._triggerAttach(displayedViews);
      }

CollectionView#_addChildView() - My preferred approach

    if (canTriggerAttach && this._triggerBeforeAttach) {
      nestedViews = [view].concat(view._getNestedViews());
      // Trigger before:attach after render to honor Region#show() lifecycle order.
      view.once('render', _.partial(this._triggerMethodOnAll, nestedViews, 'before:attach'), this);
    }

    this.renderChildView(view, index);

    if (canTriggerAttach && this._triggerAttach) {
      nestedViews = [view].concat(view._getNestedViews());
      this._triggerMethodOnAll(nestedViews, 'attach');
    }
    if (this._isShown && !this.isBuffering) {
      Marionette.triggerMethodOn(view, 'show', view);
    }

...

  // render the child view
  renderChildView: function(view, index) {
    view.render();
    this.attachHtml(this, view, index);
    return view;
  },

CollectionView#_addChildView - One alternative without "once render"

    this.renderChildView(view, index);

    if (canTriggerAttach && this._triggerAttach) {
      nestedViews = [view].concat(view._getNestedViews());
      this._triggerMethodOnAll(nestedViews, 'attach');
    }
    if (this._isShown && !this.isBuffering) {
      Marionette.triggerMethodOn(view, 'show', view);
    }

...

  // render the child view
  renderChildView: function(view, index) {
    var canTriggerAttach = this._isShown && !this.isBuffering && Marionette.isNodeAttached(this.el);
    var nestedViews;
    view.render();

    if (canTriggerAttach && this._triggerBeforeAttach) {
      nestedViews = [view].concat(view._getNestedViews());
      // Trigger before:attach after render to honor Region#show() lifecycle order.
      this._triggerMethodOnAll(nestedViews, 'before:attach');
    }

    this.attachHtml(this, view, index);
    return view;
  },

CollectionView#_addChildView - Second alternative without "once render"

    var triggerBeforeAttach = canTriggerAttach && this._triggerAttach;
    this.renderChildView(view, index, triggerBeforeAttach);

    if (canTriggerAttach && this._triggerAttach) {
      nestedViews = [view].concat(view._getNestedViews());
      this._triggerMethodOnAll(nestedViews, 'attach');
    }
    if (this._isShown && !this.isBuffering) {
      Marionette.triggerMethodOn(view, 'show', view);
    }

...

  // render the child view
  renderChildView: function(view, index, triggerBeforeAttach) {
    view.render();

    if (triggerBeforeAttach) {
      this.triggerBeforeAttachOn(view);
    }

    this.attachHtml(this, view, index);
    return view;
  },

...

  triggerBeforeAttachOn: function(view) {
      nestedViews = [view].concat(view._getNestedViews());
      // Trigger before:attach after render to honor Region#show() lifecycle order.
      this._triggerMethodOnAll(nestedViews, 'before:attach');
  }

I prefer my approach for a few reasons:

• The code within _addChildView looks much more analogous to the Region#show() code, with before:attach triggering defined in the same function as attach. I think it's more understandable to see before:attach followed by attach without jumping function calls.
• renderChildView is currently a simple method with zero logic and no event lifecycle handling, and I would like to keep it simple if possible. _addChildView is already complex, so continuing to manage complexity there is consistent.
• renderChildView is overridable, so if we place logic dependent on private variables and private method calls within it, then those overrides may break unexpectedly in the future if we change those private variables. We would have to promote those private variables (and method calls?) to public to avoid that, thus increasing our API footprint.
• Anyone currently overriding renderChildView would need to update their override to gain before:attach if we go with the alternative.
• The once('render' handler is executed synchronously following render, and obviously will clean itself up, so I don't see a problem here.

I can see two issues:

• This implementation will break silently on one condition: If a developer overrides renderChildView and does not ultimately fire render on the child view, then before:attach will not fire. It may not be obvious that the developer needs to preserve the event lifecycle of the view in order to gain before:attach. The developer override responsibility might be more explicitly obvious what if we force the developer to handle before:attach. In response to this issue I would argue increasing our API footprint and introducing logic to the renderChildView method is more problematic overall.
• once('render' is a bit confusing and not something we are doing anywhere else in CollectionView, possibly the rest of core (needs research). I would reply that the alternative is more confusing for both core maintainers and any developer who overrides renderChildView.

I added a third option that solves some of the problems, but not all of them.

I've thought a lot about this, but you all might see something I'm missing. Thoughts?

[ianmstew]

All, this could be the final commit if you like the looks of my once('render'. I gave two alternatives as well. Tell me what you think. Once we're good, I will squash. @jasonLaster you'll like that I swapped out for Marionette.triggerMethodMany()--I'm loving it.

[paulfalgout]

This is looking good. It's painful to see all of the duplication between addChildView and addEmptyView 😭 but obviously not the issue at hand.

So one thing to consider. renderChildView is not a documented function. As such I don't think it's really "public". Should it be? Perhaps we just need to add the _ and if so, would that improve the situation?

[ianmstew]

Yes, addEmptyView/addChildView is eye watering :( One of these passes through CollectionView we should refactor that.

Hmm, adding "_" would simplify things. I would change it to _renderChildView: function(view, index, triggerBeforeAttach) in order to consolidate the triggerBeforeAttach logic at the caller level.

I have no problem with this approach, and we can skip the once business that way. attachHtml is probably the more useful override. I have heard of gitter devs using the existence or non-existence of a preceding underscore to guide their "hacking", so those are the only ones would need to adapt, but that's a bit of a fringe case. We could note it in the changelog.

[paulfalgout]

Yep if this were for a minor release the _ would make me more nervous :-)

[ianmstew]

As for moving this configuration to AbstractView, it's a bit complicated actually. All this code is to allow the CollectionView to behave like a Region only when it has already been shown by a Region, a precedent set by how the show event on child views is handled. That special case probably shouldn't move up to AbstractView unless we end up completely refactoring how attach is handled.

Speaking of refactoring, the huge logical interdependency between CollectionView and Region that I mentioned doesn't sit well with me, and I want to consider a better architecture for giving CollectionView region-like behavior without all this special case handling.

[ianmstew]

Ahhhh next is the next major release ;) Got it. I was looking at the extinction of ItemView/LayoutView and wondering, ha.

Since this is a bugfix I figured it would go out in a patch release, but no?

[paulfalgout]

Ah I see.. well that'd need to be a PR against 2.4.2 at the moment.. @jasonLaster ? thought on if this should be a patch or just added to next? Might affect the
_renderChildView decision.

[jasonLaster]

Hmm, I'd prefer to have it added as a patch because re-attaching an itemview on re-render is an important consideration. With that said, this is a big patch, so I could see the case for just doing it right for 3.0. @cmaher, @ahumphreys87 how do you feel as well?

[ahumphreys87]

I'd quite like to see this in 2.4.2 if it isn't too far away, there a couple of fixes in that branch waiting for release already.

If we do this are we commited to onDetach for 2.x? If so maybe wait for 3.0

[jasonLaster]

Why would this commit us to onDetatch in 2.x?

How about this, let's get this branch onto next and then do a separate PR for 2.4.2, would be nearly identical, just nonbreaking.

[ianmstew]

@jasonLaster I agree. Rebasing onto 2.4.2 will eliminate some forward-thinking code, so I like the idea of keeping it then making a backwards-compatible version for 2.4.2. (Marionette.triggerMethodMany(), for example).

Okay, let's sum this up so we can ship.

• For next, let's go with @paulfalgout's suggestion and make renderChildView() private (_ renderChildView) and add before:attach triggers into it.
• For the 2.4.2 cherry-pick, go with once('render', and of course remove any dependencies on next code.

Thumbs up?

[jasonLaster]

👍 thanks for consolidating

[ahumphreys87]

👍 go go go :)

On Monday, 13 April 2015, Jason Laster notifications@github.com wrote:

[image: 👍] thanks for consolidating

—
Reply to this email directly or view it on GitHub
#2481 (comment)
.

[ianmstew]

On it!

[ianmstew]

Hey all, hopefully this is a wrap. A few notes:

• Went with @paulfalgout's suggestion to make renderChildView private so it could contain some private methods calls and lifecycle event firing and yet avoid once('render'. I had to remove one test that depended on renderChildView, since I figure we don't test private methods directly.
• Removed addEmptyView and generally made empty view showing much more consistent with child views.
• See what you think of DeepEqualCollectionView in the spec. Some tests break on sinon's deep equals when initializing the region manager in child views, so a previous commit disabled _initRegionManager. However, I had to add it back in since attach/beforeAttach errors without it. It's unfortunate we have two versions of CollectionView because sinon can't deep equal properly, but that's the best I could come up with.

Once this is merged I can cherry-pick onto 2.4.2. That will take some modification, but with the tests intact hopefully won't be too bad.

[ahumphreys87]

👍 so pleased addEmptyView is gone!

[ianmstew]

Realized I could clean up _addChildView further and improve its method signature; I moved before/after add events up to the calling functions, showEmptyView and addChild.

[jasonLaster]

👍

[jasonLaster]

Great work Ian!

[ianmstew]

Thanks, guys!

[jasonLaster]

no, thank you!