adds Context#execute and passes payloads as parameters instead of single...

[creynders]

... object

Proposed changes:

1. Adds an execute method to the context, to allow direct sequential synchronous execution of commands:

   context.execute([FooCommand, BarCommand], 'baz', 'qux'); //last two are payload

2. Modifies how payloads are passed to commands and callbacks:
   in case of

   context.dispatch('foo', 'bar', 'baz', 'qux');

   • commands are injected with an event object with following structure:

     {
         name: 'foo',
         data: [  'bar', 'baz', 'qux' ]
     }

   • commands also receive the payload as arguments to the execute function:

     function execute(bar, baz, qux)

   • callbacks receive the payload as arguments, just as a command's execute function

3. Throws concrete error when a command has no execute method

Then, a bit of the reasoning:

I opted for event with name and data to keep the modification as small as possible. I.e. if you'd like to update your <=0.6.2 commands to this version, "all" you'll need to do is swap this.eventName with this.event.name and this.eventData with this.event.data

A major breaking change is the fact that event names are no longer passed to callbacks. Do you think this is a problem? Commands still have access to the event name through the event object, but views won't. Would this be something you can live with? Personally I never, ever need event names in my handlers, but YMMV.

BTW I had to drop the blanket coverage treshold to 97, otherwise it failed. Do you think this is due to the introduction of the pseudo-private _execute method?

see #18
see #20

[creynders]

Forgot to add that obviously I still need to update the readme. I haven't already, since I wanted to see how this plays out.

[geekdave]

Awesome... I think this is the beginning of the first big discussion about what Geppetto's philosophy really should be. I had my own ideas, based on my "translation" of Robotlegs into JavaScript. But with some of the changes that you mentioned made it into RL 2.0 (after some debate) I'd like to open up my mind to these new possibilities. We've been using Geppetto as-is with no problems within my company, but as some smart folks have stated, To build something truly reusable, you must convince three different audiences to use it thoroughly first.

So... on to the debate!

1. Adding an execute() method to the Context

I take it this was influenced by a RL 2.0 feature? I need some convincing as to why this doesn't break the delicious decoupling that Geppetto currently promotes. Let's take the case of an app that has a "Search" feature.

Currently, when my view receives a click event from the browser on its "Search" button, the view has one job, and one job only: to dispatch a SEARCH application event onto the context. The view's job is simply to report that something happened but not to dictate what action should take place. Indeed, it is the Context's job to map that SEARCH event to RunSearchCommand (which talks to the server).

Then in the future, we get a new requirement to have a "Recent Searches" area in a separate view. Thanks to this decoupling, the "Recent Searches" view can listen for the SEARCH event on the Context, and populate itself with the info from each search.

Now, let's say that your new feature was available. Perhaps there is a use case for this that still promotes good architecture. But I'm concerned that context.execute() would be abused, and lead to tight-coupling of views to commands. If the search view had instead called context.execute(RunSearchCommand) directly, it would have been more difficult to implement the "Recent Searches" feature in the future, since the recent searches view would have no way of knowing that a search was performed.

2. Changing event payloads from maps to arrays

Currently, an event payload is a map (an Object) with keys and values. This allows us to dispatch an event like:

context.dispatch( 
    "search", 
    { 
        firstName: "bruce", 
        lastName: "wayne" 
    } 
);

Now let's say we have a command set up to listen to the search event like so:

function execute() {
   console.log("First name: " + eventData.firstName);
   console.log("Last name: " + eventData.lastName);

No assumption is made about the order of the payload properties, since they are referenced by key. With your change, we would instead write this as:

context.dispatch( "search", "bruce", "wayne" );

It's cleaner and less verbose this way, but I'm worried it's less flexible for the consumer of the event. Let's take the above example command, rewritten with this new payload structure.

function execute(firstName, lastName) {
   console.log("First name: " + firstName);
   console.log("Last name: " + lastName);

Let's say that my search form grew bigger and bigger and now has 50 fields in it. With the old "map" style, I could pass an event payload and only populate the properties that the user actually searched on. So if they only searched for 3 fields, my payload would only have 3 properties. But with the "array" style, I would need an execute() method with 50 arguments... and it might be easy for the ordering to get out-of-sync between the dispatcher and the listener.

3. Event names no longer passed to callbacks

For single-context apps, I don't think it's a problem. But for multi-context apps, it will be problematic. A lot of my Geppetto apps borrow from @joelhooks Modular Robotlegs extension, which has the notion of "gatekeeper" mediators that forward events from one Context to another. In ActionScript, since we were dealing with concrete Event types, with a clone() method, it was trivial to re-dispatch an event onto another context, to "pass the message along". But with JavaScript, our events are simple objects, and so we need both the object name and the payload to manually trigger another event on another context.

One example of this is a parent Context that manages the "shell" of an application, including swapping out the main View when a new page is displayed. The shell context knows to do this swapping when it receives a NAVIGATE event. But what if the shell gets a navigation event for a different state of the page that's already being displayed? Instead of trashing the current view, the shell context should instead forward the navigation event on to the current view so it can display the proper state. To do this, it needs both the event name and payload to trigger a new event on the child context.

BTW I had to drop the blanket coverage treshold to 97, otherwise it failed.

Hmm, not sure. If you start a local server and navigate to http://localhost:8000/specs/ scroll to the bottom for the BlanketJS report, and click on the filename, you will see non-covered lines highlighted in red. So it's easy to spot where coverage is missing.

Looking forward to debating further, and hearing your use cases!

[creynders]

Heheh, I love this kind of debates.

Context#Execute

Perhaps there is a use case for this that still promotes good architecture.

Yes, when called from commands.
Instead of having to do this:

Context.extend({
    commands:{
        'qux': [
            FooCommand,
            BarCommand,
            BazCommand
        ]
    }
})

You can do:

//wiring
Context.extend({
    comands:{
        'qux' : QuxCommand 
    }
});

//QuxCommand
function execute(){
    this.context.execute([
        FooCommand,
        BarCommand,
        BazCommand
    ]);
}

This is useful in cases where FooCommand, BarCommand and BazCommand constitute a single, logical sequence, yet still need to be able to be triggered separately as well. Added benefit is that if you want to trigger the sequence through several events, you won't be copying the commands array. It's more robust since if the sequence needs to be changed, you only need to change it in one place.

However

But I'm concerned that context.execute() would be abused, and lead to tight-coupling of views to commands.

Quite right. Hadn't thought of that. That is definitely a serious concern. To me it's so evident not to use it in views, that I hadn't realized people will be abusing it.

Maps to arrays

Let's say that my search form grew bigger and bigger and now has 50 fields in it.

Nothing prohibits you from using a map, just as you did before. I too tend to use VO's, but there are times when it is easier and more flexible to simply pipe all arguments through.

Event names no longer passed to callbacks

But for multi-context apps, it will be problematic.

Ah yes, of course. Hmmm... need to think on that. Obviously it's possible to pass the event name as a parameter, but I'm not really fond of that.

[geekdave]

Yes, when called from commands.

Aha! I definitely see the value there.

To me it's so evident not to use it in views, that I hadn't realized people will be abusing it.

It's been my experience that developers will often find the path of least resistance, even if it means ignoring best practices. So better not to give them the gun, instead of advising them not to shoot themselves in the foot.

Here's a crazy idea. What if Geppetto injected the command with its own executeOther method, after the command gets instantiated? So that you could do:

function execute() {
    this.executeOther([
        FooCommand,
        BarCommand,
        BazCommand
    ]);
}

This special executeOther method would therefore only be accessible to commands, and it would pass-through to the backing context. Not sure if executeOther is the best name... but I guess we can't use execute since it's used by the command itself.

[geekdave]

Regarding the chaining of commands, and executing one command from another, my team had some concerns:

Almost everytime we use a command it's generally to make a server call. If a command calls another command sequentially, it makes more sense to have a single server call that executes the same business logic, which atleast saves you a roundtrip, unless that the two commands callout to different domains(json/jsonp). I also see that this chaining can lead to some problems, like for example, how are the errors traced back to the caller?

...and...

I think it is a rare use case and cannot be implemented without
restricting how a command terminates.

The termination of a command can have multiple definitions. For example:

command1.execute = function(callback){
  doSomething();
  Ajax().success(callback);
  context.dispatch(I_AM_FINISHED);
}

If we chain command1 with command2, when do we execute command2? It can be:

1. after execution of these 3 lines of code.
2. after asynchronous callback is executed.
3. after the FINISH event is fired.

I don't see the value of sacrificing flexibility for the connivence of
a rare use case.

So now I'm leaning more towards not including this feature. @creynders do you have any real-world scenarios where this would be useful? I understand the hypothetical advantage, but just wondering how this would manifest itself in the real world.

[creynders]

It's been my experience that developers will often find the path of least resistance, even if it means ignoring best practices.

Agreed. It's why we don't advertise some of the possibilities in RL, but merely mention them, figuring that only people who know the framework and its paradigm well enough will understand and realize the potential use cases.

Here's a crazy idea. What if Geppetto injected the command with its own executeOther method, after the command gets instantiated?

That's definitely an option. Maybe executeCommands is more appropriate?

do you have any real-world scenarios where this would be useful?

I think we use commands differently. To me commands perform no real work, they assemble, construct and relay.
The first valid use case that comes to mind is application bootstrapping: a Startupcommand which calls BootstrapViewsCommand, BootstrapModelscommand, ConfigureFooModelCommand, etc.
But I'm pretty certain I'll encounter enough situations where its warranted. Obviously this can be solved in a myriad of ways, it's not a deal-breaker.
Whether it's a rare use case, I'm not certain. I haven't used BB and G long enough to say one way or the other. Could be you're quite right.

If we chain command1 with command2, when do we execute command2?

Synchronously, i.e. immediately. When I want to chain asynchronous operations I use a finite state machine or promises.

I don't see the value of sacrificing flexibility for the connivence of
a rare use case.

I don't see how this would sacrifice flexibility? On the contrary.

[creynders]

So, I've been thinking this through in the past weeks. The more I think about it, the more convinced I get that my initial gut feeling revolves around symmetry and paradigm concordance, more than the above mentioned benefits.

I think the whole events thing should be brought back to basics and the abstraction layer on top of Backbone.Events should be removed:

context.trigger('foo', 'bar', 'baz', 'qux');

Commands' execute methods only receive the parameters 'bar', 'baz', 'qux', however an event object is constructed and injected into commands as well:

{
    name: 'foo',
    data: [  'bar', 'baz', 'qux' ]
}

Then regarding the redispatching/relaying of events, IMO there's always a better, more elegant solution. But if really necessary we could provide an extra method:

var handler = function(eventName, bar, baz, qux){
}
context.relay('foo', handler);

And I'm convinced now, we don't need Context#execute.

Now there's several reasons why I think we should remove the abstraction layer on top of Backbone.Events:

1. easier portability: it's easier to plug Geppetto into an existing project -or- (let's hope that doesn't happen often) remove it from an existing project
2. recognizability: people are already used to the Backbone.Events messaging style. I too was first put off when just starting out with Geppetto because I thought it implemented its own eventing system on top of the existing one.
3. symmetry: there's no difference between writing context event handlers and view event handlers

[geekdave]

@creynders Is this PR obsolete now?

[creynders]

@geekdave The PR definitely is. But I would like to continue the discussion on the benefits/disadvantages of removing the abstraction layer on top of Backbone.Events. Maybe a separate issue?

[geekdave]

@creynders Sorry for the delay in responding. We are just now getting around to refactoring our code to use the new DI API.

The PR definitely is [obsolete]

Will close this one for now...

I would like to continue the discussion on the benefits/disadvantages of removing the abstraction layer on top of Backbone.Events. Maybe a separate issue?

Agreed. Please log a separate issue with your proposal and we can discuss it there.