GeppettoJS / backbone.geppetto

Bring your Backbone applications to life with an event-driven Command framework.
http://geppettojs.github.com/backbone.geppetto/
MIT License
203 stars 28 forks source link

Pass context, event name and data to command constructors #44

Closed creynders closed 10 years ago

creynders commented 10 years ago

To allow for simple commands w/o execute methods I think it would be a good idea to pass the context, eventName and eventData to the command constructors.

I added a spec, but can't get the tests to run (and don't have the time right now to check what's wrong), when I run npm test I get an error:

~~>Testing: specs/index.html Warning: PhantomJS timed out, possibly due to a missing Mocha run() call. Use --force to continue.~~

geekdave commented 10 years ago

Welcome back to Geppetto-land, @creynders !

I'm trying to understand what you mean by "simple command." Are you trying to reduce boilerplate by encapsulating a command's business logic entirely in the constructor, as opposed to a separate execute method?

I do concede that since commands are single-use (instantiated, executed, and destroyed automatically by Geppetto) that having a named execute method does seem a bit boilerplate-y. But I also like the usability of the logic within the execute method having access to nicely-named injected properties like eventName and eventData. Relying on an ordered array ctorOptions for this seems a bit hackish.

Could you help me better understand how this would make life easier for Geppetto command-writers?

creynders commented 10 years ago

Yeah, sorry, I was kind of terse with my explanation I suppose.

I like to keep my commands as simple and small as possible, i.e. I would like to skip the execute method in most of them. That's why I'd appreciate it if the context and event data would be injected as arguments to the constructor function.

The ctorArguments is just a convenience inside the spec definition, that's definitely not the way I'd use them. Rather like this:

module.exports = function SetupI18N( context, eventName, eventData ){
    $.i18n.init( {
        lng : "nl-BE",
        ns  : {
            namespaces : ['common'],
            defaultNs  : 'common'
        }
    }, function( t ){
        context.vent.dispatch("bootstrap:SetupI18N:completed");
    } );
}

As you can see the command doesn't need full-blown dependency injection per se, it just needs to be able to dispatch an event to the system (and maybe access the event data), that's why it would easy to have them inside the constructor function.

geekdave commented 10 years ago

Ok, I'm sold. Would you mind including a doc update for this in the PR as well?

creynders commented 10 years ago

Will do!