Adding region aliases into childEvents hash

[msiconolfi]

Preliminary enhancement for supporting region aliases in the View.childEvents hash, as requested in Issue 2763.

• Moved layout specific logic out of AbstractView and into View object, and simply calling layoutView.triggerChildEvents(eventName, this, args); from inside AbstractView._triggerEventOnParentLayout; to me this seems to separate the logic a little better, and also provides a hook for subclasses to override if needed.
• Added region alias detection to determine if the current view is the one specified in the childEvents hash, non-region aliases should behave the same as in earlier releases.

[jasonLaster]

nice @msiconolfi!

I left a comment in #2783. But, i wonder if @region.regionName could be replaced with @regionName.

Also, not sold on the value but I'd defer to the group here.

Lastly, if we used the generic @alias maybe we could re-use the normalize methods in utils even more?

More stuff, tests are great. Mind adding some docs

[paulfalgout]

I think I'm 👎 on this.

While it follows the established pattern, I'm not confident in the current implementation.

My concern in general is that we are doing a lot of work for each event a child throws.. and those child events are already pretty noisy. Seems like the @region childEvents could be analyzed before the view is shown one time and then those event hashes are managed separately?

I'm not sure exactly the solution, but in general I think we need to find ways to reduce trigger weight and to reduce event noise.

[jasonLaster]

@paulfalgout i think you're right. We do that for the other aliases

[msiconolfi]

@paulfalgout I agree the _.each is needlessly ran for every childEvent. I could do as you said and analyze the before the view is shown the first time. However, it seems like this feature may not ultimately desired.

If you think that this might be a viable feature to eventually release, then I can work on a better approach. Could others weigh in on whether adding @region aliases for childEvents is something we would want in the library one day?

[paulfalgout]

Hmmm.. it may very well be a viable feature.. I personally think childEvents and childTriggers are an improvement to and should replace the auto-proxying of all of the 'onChildviewBeforeRender': 'magic', but I'd like to see a cheap lookup for what events should be proxied.

Like someday a refactor would be to remove https://github.com/marionettejs/backbone.marionette/pull/2819/files#diff-7f9d7745f16bb572e41cb88f8eaf7734R246

[jasonLaster]

ping @msiconolfi hope you have a chance to tackle this :)

[msiconolfi]

Still working on this. I wanted to review the library as whole to try and see how to best fit it in and provide the cheap lookup. I should have a second implementation in a couple of days.

[jasonLaster]

Nice. What was your opinion of the library as a whole? Everything is busted? Not so bad :)

Also, did you see we landed the new ES6 build and modules yesterday? AKA everything is different :P

[msiconolfi]

@jasonLaster In general, everything is nicely laid out, I was just trying to get a handle of the differences between next and the patch branches.

I'll take a look at the ES6 build and modules today. But it sounds exciting so far.

[msiconolfi]

Regarding @paulfalgout comment "Seems like the @region childEvents could be analyzed before the view is shown one time and then those event hashes are managed separately?". This would work fine for when childEvents is defined as a hash but when it is defined as a function returning a hash, this would not work. Since the function returning a hash would be dynamic we wouldn't know until it was executed and wouldn't be able to take advantage of any pre-show lookup generation.

Any ideas of how we could still make this a cheap lookup, at this point it may be too expensive for it to be a useful feature.

We could possibly add a regionEvents attribute which only supports a hash, but that might not fit the pattern of childEvents, collectionEvents, and modelEvents of allowing either a hash or function returning hash.

[jasonLaster]

Should be fine. The childEvents function would be executed on view instantiation and saved to an indtance prop we can access.

[jasonLaster]

☎️ @msiconolfi! How goes it?

[msiconolfi]

Still working on it. I'm not in love with my current implementation, but would appreciate any feedback.

Here is the basic flow:

• Upon calling View.showChildView() regionEvents will be setup using the newly added View._bindRegionEvents(regionName) method.
• This will iterate over the normalized childEvents and parse out any region aliases that match the regionName.
• Any matching events will be bound directly to the childView using the listenTo() method.
• The events will then be stored in a private variable this._regionEvents so that they can eventually be unbound upon: new region show, or a region destroy.

I still need to add View._unbindRegionEvents() to a couple more places but not sure I am totally in love with this approach yet.

A couple things I don't like.

• I had originally wanted to do the lookup in my View.triggerChildEvents() method, however there was no easy way to determine in which region(s) the childView belonged so I could do the lookup on View._regionEvents. I could still continue this way but then I would also have to create a mapping between childViews and the region(s) they belonged. The difficult part was supporting the ability for one child view to possibly be bound to more than region, which may not be a reasonable expectation. If we lax that requirement it probably wouldn't be too bad to create a lookup that will map views to region.
• As an alternative the problem above, I decided that I could use listenTo to bind the events directly when the region was showed, since at that time I know the region name. For most cases the events will be unbound on the child view's destroy(), however if the view has the preventDestroy flag set then we need to manually clean up the regionEvents upon showChildView / destroy() of the View. This was the reason I had created the View._unbindRegionEvents() method.

[jasonLaster]

Definitely going in the right direction. I like bind and unbind w/ listenTo.

Mind rebasing. We've been busy :)

[JSteunou]

Is this going v3? no milestone on original issue #2783

[paulfalgout]

v3.x :-)

[msiconolfi]

I can pick this up this week, sorry this fell off my plate. I just need to rebase and then re-work the bind/unbind. I should be able to get something up and running this week to review.

Thanks for the nudge.

[paulfalgout]

@msiconolfi no pressure here at all. Thanks for being up to working on it! We might be in the weeds a bit soon with the v3 release candidate. This could certainly land at any point, but take all the time you need 👍

[rafde]

would _.toArray help shorten this line?

[maniator]

Actually since we are using es6 just use spread would be better.

Like so: https://github.com/marionettejs/backbone.marionette/pull/2819/files#r56397920

[scott-w]

Is this being subsumed into #3157?

[paulfalgout]

This is pretty outdated now. For now it seems like the best course is to close this and refer to it if anyone picks up the open issue.