Add method to trigger events and corresponding method

[mxriverlynn]

Another bit I've been thinking about for a while, and finally put together... I want to get some feedback on this to see if it's really valuable or not.

Many of the events in Marionette's views have corresponding methods. For example, "render" event has "onRender" method. But many of the corresponding methods are named inconsistently: "before:close" before has "beforeClose" method ... note the missing "on" in front of that.

To clean up the inconsistencies and to facilitate additional "onWhatever" methods that are called when events are triggered, I added a Marionette.triggerMethod function and attached it to the base View. I also updated all calls to trigger inside of every Marionette view so that it now calls triggerMethod instead.

This has a small effect of cleaning up some code, but a larger effect of making all triggered event + method combinations more consistent, and making the corresponding method available for all triggered events, not just the ones we had coded explicitly, previously.

A few bits on how this works:

When an event is triggered, the first letter of each section of the event name is capitalized, and the word "on" is tagged on to the front of it. Examples:

• triggerMethod("render") fires the "onRender" function
• triggerMethod("before:close") fires the "onBeforeClose" function

All arguments that are passed to the triggerMethod call are passed along to both the event and the method, with the exception of the event name not being passed to the corresponding method.

• triggerMethod("foo", bar) will call onFoo: function(bar){...})

I'd like some feedback on this idea and implementation. It does break some existing function names, such as "beforeClose" which is now "onBeforeClose". But I think the small breaking change is worth the consistency change. There may be some additional updates for methods like "onShow" or others that might need to be examined / changed as well.

[codingluke]

I like it! I'm a big fan of scripted conventions, they are the only ones I will not break on a foolish day. The only thing to consider is to check first if the function onFoo is defined before onFoo gets called. THX

[andyl]

I like the idea of tying events and method names together. +1 for consistency.

Re: breaking the API - could you add a method alias which prints a deprecation warning to the console ??

[mxriverlynn]

@tbhodel right - i have a basic check here https://github.com/marionettejs/backbone.marionette/pull/265/files#L9R22 but i need to make that check for _.isFunction to be sure. good catch.

@andyl yeah, that can be done.

[jsoverson]

This looks great to me, anything to consolidate like logic.

On Tue, Oct 2, 2012 at 9:36 AM, Derick Bailey notifications@github.comwrote:

@tbhodel https://github.com/tbhodel right - i have a basic check here
#265https://github.com/marionettejs/backbone.marionette/issues/265but i need to make that check for
_.isFunction to be sure. good catch.

@andyl https://github.com/andyl yeah, that can be done.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/265#issuecomment-9077456.

[ccamarat]

Being a huge fan of consistency I'm loving this change. I am curious about two things:

1. I notice that the close event on View isn't standardized; it triggers 'Close' when it closes, while all other actions trigger some form of a past-tense event, e.g. 'Closed'.
2. Not all 'actionCompleted' events have a corresponding 'before' event; generally these events don't seem like places where there's value in such thing, but to improve consistency and adhere to the principle of least astonishment is it worth the extra overhead to trigger events on actions like 'beforeItemAdded'?

As an aside, the naming convention for before and after events, 'beforeAction' / 'actioned', strikes me unbalanced; 'beforeAction' / 'afterAction' seems like it would have been a little cleaner.

[mxriverlynn]

@ccamarat can you open a new issue ticket w/ exactly that same text? I'd like to track that as a separate but related issue. thanks.

[ccamarat]

Done. Sorry for the delay.