Add first class support for Backbone.Radio in Mn.Object #2431

benmccormick · 2015-03-19T04:26:06Z

This is a very initial PR to try and handle #2324. It needs more tests, any documentation at all, and the code could probably be cleaned up considerably. But I wanted to keep that issue moving and give everybody a concrete proposal to discuss.

This PR adds support to Mn.Object and its subclasses for the following API:

radioEvents: {
  'app start': 'onAppStart',
  'app finish': 'onAppFinish',
  'books start': 'onBooksStart',
  'books finish': 'onBooksFinish',
},

radioCommands: {
  'app doFoo': 'makeFooHappen',
},

radioRequests: {
  'resources bar': 'getBar',
},

The biggest change that it would involve is making Backbone.Radio a dependency of Marionette. In this PR, I've made it a hard dependency and something that the UMD modules try to pull in automatically. I could also see this being written in a way though that only activated the feature if Radio was present on the Backbone object.

jasonLaster · 2015-03-19T13:53:04Z

src/object.js

@@ -37,5 +37,62 @@ _.extend(Marionette.Object.prototype, Backbone.Events, {
  bindEntityEvents: Marionette.proxyBindEntityEvents,

  // Proxy `unbindEntityEvents` to enable unbinding view's events from another entity.
-  unbindEntityEvents: Marionette.proxyUnbindEntityEvents
+  unbindEntityEvents: Marionette.proxyUnbindEntityEvents,


I don't want to add too much more work for you, but I think this deserves it's own file. Maybe src/radioHelpers.js

src/radioHelpers.js

I can get on board with that. It certainly is doubling the size of Mn.Object right now. So you're suggesting just leaving the _.proxyRadioHandlers call in the constructor, and then adding a separate extend call in a different file?

Nah, I'm suggesting adding two functions: Marionette.bindRadioHandlers(context) and Marionette.unbindRadioHandlers(context). I think we could use these two functions in the Object class, but atleast we don't clutter Object source w/ tons of radio knowledge. And, other external classes get this logic for free in 3.0

paulfalgout · 2015-03-20T05:24:22Z

So I think maybe we need to take a step back on this for a sec and look at either Radio additions or Marionette/Radio helpers that need to be introduced prior to this work.

One concern in this PR is that we're using on/off for the listeners.. correct me if I'm missing something, but this sets a bunch of on listeners on a channel, and when it gets cleaned up, off is called on the channel, but you may have other listeners out there getting inadvertently cleaned up. This is much less of a concern with command/reply as you only have one actor..

But so we should really be switching to listenTo/stopListening which fortunately Object already handles quite well. Similarly I would suggest adding replyTo / complyTo helpers for Object such that the object's destroy only needs stopReplying/stopComplying on itself to handle any reply/comply events registered with itself, rather than on the channel directly.

Plus any inline Object.replyTo Object.complyTo would be automatically cleaned up by the Object.destroy as well.

Second,
Is it really the common case that any object would want to do this for multiple channels? For my use cases, I generally have one channel that the object is mainly concerned with, and occasionally another or two that I might use for a one-off here or there.

I think it would be better to suggest a primary channel for an object, giving the user the freedom to replyTo / complyTo other channels inline as necessary. I also think this would simplify the implementation quite a bit.
However if necessary it seems reasonable to allow for the 'channelName replyName' : 'handler' but for almost all of my known use-cases I'd be duplicating the same channelName over and over again, rarely using an additional channel. If that's the case would anyone be interested in a primary channel that is implied such that:

  channelName: 'someChannel',
  radioEvents: {
    'some:event', 'someHandler',
    'otherChannel other:event', 'someOtherHandler'
  }

etc.

paulfalgout · 2015-03-20T05:27:30Z

Ah I jumped the gun a bit.. Radio is working on replyFor / complyFor here: marionettejs/backbone.radio#157

TL;DR for my last post. Get this Radio API in first, and utilize it for Object.

jasonLaster · 2015-03-20T14:55:22Z

haha - @paulfalgout. This stuff is confusing, no?

I definitely like the idea of RadioHelpers and radio helper tests, which will expose the api we offer. Lets try and get the smallest thing in which in my mind is radioEvents, radioRequests, radioCommands and the bind and unbind methods that set them up.

paulfalgout · 2015-03-20T15:16:03Z

I don't know how you accomplish radioEvents, radioRequests, radioCommands without them being leaky or overly destructive without better base helpers that attach listeners to the object instead of the channel.

jfairbank · 2015-03-20T15:29:58Z

src/object.js

+  _unproxyRadioEvents: function() {
+    _.each(this._radioChannels, function(channel) {
+      _.each(this._radioTypes,function(commands) {
+        Backbone.Radio[commands.stopMethod](channel,this);


Passing in this as the second argument will prevent unbinding I believe. The second argument needs to be the callback and then the context. Obviously, listenTo/stopListening and the forthcoming command/reply counterparts would be preferred here. Otherwise, we'll need to track the callbacks we add, which is really just reimplementing listenTo-type behavior.

Yep. I think I actually need to add more arguments here, Backbone events isn't smart enough to figure out that this is supposed to be the context if you only pass one argument. So this is broken.

But thats the general idea of what I'm trying to achieve. A small fix will prevent over-unsubscribing on everything but manually subscribed methods on the same object. Like @jfairbank said, if we want to be airtight we will need to keep track of the callbacks or get the updated radio functions. It's hardly a disaster as is though, especially since the _unproxyRadioEvents stuff should only be run in the constructor and on destroy anyway. It wouldn't interfere with existing radio usage unless somebody called it manually.

marionettejs/backbone.radio#162 There's also a confusing issue with radio around clean-up not being as selective as it should.

@jmeas you anywhere where you could weigh in?

benmccormick · 2015-03-20T15:34:37Z

One concern in this PR is that we're using on/off for the listeners.. correct me if I'm missing something, but this sets a bunch of on listeners on a channel, and when it gets cleaned up, off is called on the channel, but you may have other listeners out there getting inadvertently cleaned up.

I don't know how you accomplish radioEvents, radioRequests, radioCommands without them being leaky or overly destructive without better base helpers that attach listeners to the object instead of the channel.

on and off accept a context argument that only deletes events and commands in the context of that object. See: http://codepen.io/ben336/pen/zxyPQj I'm currently not doing this right in the code. The whole unregistering stuff got written too quickly and is screwed up. I'll clean it up and resubmit soon. But basically you can have pretty fine-grained control of what you're unsubscribing from. I was trying (a little too carelessly sadly) to deregister only events in context of the current object. That would possibly remove any manual listeners on that object, but wouldn't interfere with anything else.

benmccormick · 2015-03-20T15:47:47Z

So, thanks for the feedback everyone. I'm going to take some of that back and try to improve the PR. Specifically:

Clean up the _unproxy code and fix my dumb typos
add tests to make sure we're not over-unsubscribing when we do unsubscribe
Pull the radio stuff out into a separate file
Add _unproxy to destroy

I'll aim for that over the weekend and hopefully this can continue to get pushed forward.

paulfalgout · 2015-03-20T15:49:51Z

👏 thanks a ton for taking this on! I'm pretty excited to start using it actually. It will clean up a lot of boilerplate for my various services.

jasonLaster · 2015-03-20T16:23:31Z

too late @paulfalgout

https://twitter.com/jasonlaster11/status/578954960420769794

benmccormick · 2015-03-21T20:48:11Z

Ok I worked in the feedback from @jasonLaster @jfairbank and @paulfalgout.

The radio code is now in its own file. The unsubscribe stuff has been cleaned up so that it works correctly and happens in the destroy function. And there are tests to verify that and also make sure it doesn't "over unsubscribe".

The only cases I can see where this might interfere with existing code:

If a user has been manually wiring up object methods to radio events and doesn't want them to be unsubscribed when the object is destroyed.
if a user is using a _radioChannels property on an existing object (seems possible, I think the other functions being added are long enough/specific enough that they're unlikely to cause collisions)

The first one could be mitigated by doing more manual tracking of which callbacks are being attached. I don't think thats worth it.

The second could be mitigated by creating a central object => channel tracking object rather than sticking that directly on the object. I'm indifferent here, I can see advantages either way.

Other than that, the only big issue I'm aware of with this code is that it currently makes Radio a hard dependency for Mnv3. If that is not something the core team wants to do, I'll probably have to put some thought into the best way to make this an optional behavior depending on Radio availability, similar to Backbone.View.$.

(oh, and obviously this needs documentation work. Waiting on some agreement that this is a good approach before adding that)

jasonLaster · 2015-03-24T16:22:40Z

test/unit/radio-helpers.spec.js

+      this.clickStub3 = this.sinon.stub();
+      this.Object = Marionette.Object.extend({
+        radioEvents: {
+          'foo bar' : this.clickStub1


This is weird because you run into the object2 problem w/ prototypes.

The easier thing is to add the stub to the object directly.

O = extend({ radioEvents: {'foo bar': 'barHandler'} barHandler: function() {} }); o = new O(); o.barHandler = this.barHandler;

ok, I can switch that up.

@jasonLaster So after looking at this again, I don't see the problem here. I pass 2 functions directly and one by reference, to make sure that we handle both of those cases. Yes object2 shares the stub on the prototype, but I think the testing is still pretty unambiguous. I just wrote it out the other way, and it makes the code a lot more verbose and (in my mind) less clear. So I'm going to leave it unless you strongly object.

i'm cool with this

jasonLaster · 2015-03-24T16:31:10Z

Really excited about this @benmccormick!

API is solid
Left some comments on test and code
Lets add Radio as hard dep to Mn for V3.
_radioChannels is fine for V3. We can get the upgrader to check for that terrm as a safety.
not worried about over subscribing, apps can progressively switch to radioEvents etc
I'm -1 central registry. It makes sense in theory, but it's not something I'd expect w/ BB.events and other patterns.
Looking forward to docs

💯 🌴

ahumphreys87 · 2015-03-26T21:01:26Z

@benmccormick this is looking great! This is gonna be such a nice feature for v3!

- Radio added as a dependency - Mn.Object now supports radioEvents, radioCommands, and radioRequests hashes in the form `{'channel message':'handler'}` - Tests added for this feature - Initial Documentation Note that this is only adding support to Mn.Object and the Classes that inherit from it, not trying to add the same support to Backbone.View

benmccormick · 2015-04-04T03:28:10Z

@jasonLaster Addressed the comments other than the testing one, which I replied to. Added a first pass at docs.

Do we want to try to include this in Views as well? Or just Object? Behaviors and Application get it by default now, though I'm not sure commands or requests make any sense at all on Behaviors, since they have to be unique. But if they're going to get included there, probably makes sense to include them on Views as well.

benmccormick · 2015-04-04T04:44:03Z

Also, I threw together a quick library for anyone who wants to use this feature with Mn 2.1.x.
https://github.com/benmccormick/marionette-service

ahumphreys87 · 2015-04-04T21:20:55Z

@benmccormick docs looking good. Maybe provide a link to the radio docs to help users who are new

jasonLaster · 2015-04-06T14:05:33Z

👍 (one to go)

@benmccormick Lets create a separate PR for adding Radio to Views. It should be easy / un-controversial, but still nice to break up the discussions.

This looks great. Huge Huge. 3.0 contribution.

ahumphreys87 · 2015-04-06T14:18:43Z

👍 let's add the link to radio docs when we add to views

Add first class support for Backbone.Radio in Mn.Object

jasonLaster · 2015-04-06T20:06:56Z

📻 🔘 💭

Yesss!
@jmeas, it's in!

benmccormick · 2015-04-06T20:23:18Z

👍 Thanks for the feedback everyone. I'll toss up a separate PR for the Views/ more documentation sometime later this week. Is there anywhere else in this repo where I should document that Radio will now be a dependency (instead of wreqr?) Or is that more of a www thing?

jasonLaster · 2015-04-07T00:46:05Z

That's a good question. Man it'd be great to pull in radio docs.

benmccormick mentioned this pull request Mar 19, 2015

Add api for binding an object to a radio channel #2324

Closed

jasonLaster reviewed Mar 19, 2015
View reviewed changes

jfairbank reviewed Mar 20, 2015
View reviewed changes

paulfalgout mentioned this pull request Mar 20, 2015

Add BaseClass RoundingWellOS/marionette.toolkit#27

Closed

benmccormick force-pushed the radio_object_integration branch from ac723c0 to dd4bec8 Compare March 21, 2015 20:35

jasonLaster reviewed Mar 24, 2015
View reviewed changes

benmccormick force-pushed the radio_object_integration branch from dd4bec8 to a5afa20 Compare April 4, 2015 03:15

ahumphreys87 added a commit that referenced this pull request Apr 6, 2015

Merge pull request #2431 from benmccormick/radio_object_integration

f3d4fdc

Add first class support for Backbone.Radio in Mn.Object

ahumphreys87 merged commit f3d4fdc into marionettejs:major Apr 6, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add first class support for Backbone.Radio in Mn.Object #2431

Add first class support for Backbone.Radio in Mn.Object #2431

benmccormick commented Mar 19, 2015

jasonLaster Mar 19, 2015

benmccormick Mar 19, 2015

jasonLaster Mar 19, 2015

paulfalgout commented Mar 20, 2015

paulfalgout commented Mar 20, 2015

jasonLaster commented Mar 20, 2015

paulfalgout commented Mar 20, 2015

jfairbank Mar 20, 2015

benmccormick Mar 20, 2015

paulfalgout Mar 20, 2015

benmccormick commented Mar 20, 2015

benmccormick commented Mar 20, 2015

paulfalgout commented Mar 20, 2015

jasonLaster commented Mar 20, 2015

benmccormick commented Mar 21, 2015

jasonLaster Mar 24, 2015

benmccormick Mar 24, 2015

benmccormick Apr 3, 2015

jasonLaster Apr 6, 2015

jasonLaster commented Mar 24, 2015

ahumphreys87 commented Mar 26, 2015

benmccormick commented Apr 4, 2015

benmccormick commented Apr 4, 2015

ahumphreys87 commented Apr 4, 2015

jasonLaster commented Apr 6, 2015

ahumphreys87 commented Apr 6, 2015

jasonLaster commented Apr 6, 2015

benmccormick commented Apr 6, 2015

jasonLaster commented Apr 7, 2015

Add first class support for Backbone.Radio in Mn.Object #2431

Add first class support for Backbone.Radio in Mn.Object #2431

Conversation

benmccormick commented Mar 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfalgout commented Mar 20, 2015

paulfalgout commented Mar 20, 2015

jasonLaster commented Mar 20, 2015

paulfalgout commented Mar 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccormick commented Mar 20, 2015

benmccormick commented Mar 20, 2015

paulfalgout commented Mar 20, 2015

jasonLaster commented Mar 20, 2015

benmccormick commented Mar 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonLaster commented Mar 24, 2015

ahumphreys87 commented Mar 26, 2015

benmccormick commented Apr 4, 2015

benmccormick commented Apr 4, 2015

ahumphreys87 commented Apr 4, 2015

jasonLaster commented Apr 6, 2015

ahumphreys87 commented Apr 6, 2015

jasonLaster commented Apr 6, 2015

benmccormick commented Apr 6, 2015

jasonLaster commented Apr 7, 2015