marionettejs / backbone.radio

Messaging patterns for Backbone applications.
MIT License
491 stars 50 forks source link

Radio.command should support objects with handlers as strings #121

Closed jasonLaster closed 10 years ago

jasonLaster commented 10 years ago

I should be able to have handlers as strings

Radio.command('app', {'show:app': 'showApp'}, this)

I think that's because we don't use _.result for the callback. https://github.com/jmeas/backbone.radio/blob/master/src/backbone.radio.js#L123

jamesplease commented 10 years ago

hmmm...does this work for Events? That answer is the answer to whether or not I want to add this, I think.

jasonLaster commented 10 years ago

oh man - you definitely want it. It's in all of the Wreqr connect X methods src. bindEntityEvents works that way.

most importantly radio hashes should look the same as all others.

View.extend({

    modelEvents: {},
    events: {},
    channelEvents: {},

   initialize: function() {
      Radio.comply('app', this.channelEvents, this);
  }
});

oh and. if that's not enough. it's additive, so getting it in doesn't hurt anyone.

oh and if we don't do it I have to write shitty code like this:

    commands: function() {
        return {
          'show:tool': this.showTool
        }
    },

i apoligize for the barf/rant.

jamesplease commented 10 years ago

I don't disagree that it's convenient, but if it isn't supported by BB.Events, then folks might get confused by the inconsistent API. Consistency between Events/Requests/Commands is my number one priority, more so than functionality.

jamiebuilds commented 10 years ago

@jasonLaster that functionality belongs outside Radio like how Backbone's bindEntityEvents is in Backbone.View and not Backbone.Events.

jamesplease commented 10 years ago

Agreed with @thejameskyle. Gonna close this for now.

jasonLaster commented 10 years ago

Hmm, I guess this is confusing because Backbone.Events.on doesn't have this on({'show:app': 'showApp'}, this), so i'm not sure what the precident is...

jamesplease commented 10 years ago

Backbone.Events.on doesn't have this on({'show:app': 'showApp'}, this)

Yup, that's what James and I are saying. And because it doesn't, Requests and Commands won't either.

The precedent is to make the API for Commands and Requests identical to Events. With Events, you can use the object syntax, but it won't accept strings as lookup values: only actual methods. Consequently, Requests and Commands will behave in the same way.

Unless I'm missing something, I think we're consistent with the current implementation.

jasonLaster commented 10 years ago

Oops misspoke. I didn't know B.Events accepted hashes at all.

On Sunday, August 3, 2014, James Smith notifications@github.com wrote:

Backbone.Events.on doesn't have this on({'show:app': 'showApp'}, this)

Yup, that's what James and I are saying. And because it doesn't, Requests and Commands won't either.

The precedent is to make the API for Commands and Requests identical to Events. With Events, you can use the object syntax, but it won't accept strings as lookup values: only actual methods. Consequently, Requests and Commands will behave in the same way.

Unless I'm missing something, I think we're consistent with the current implementation.

— Reply to this email directly or view it on GitHub https://github.com/jmeas/backbone.radio/issues/121#issuecomment-51015456 .

Jason Laster Software Engineer Etsy

jamesplease commented 10 years ago

Heh yup. Every method on BB.Events is passed into eventsApi, which handles both the object and the string-separated arguments.