marionettejs/backbone.radio

Messaging patterns for Backbone applications.

backbone.radio's Issues

Review return statements

Quickly looking through it seems like theres some inconsistency of what is being returned when. We should look through it all and make sure its what we want.

Support for holding / retrospective events

It would be useful to have an option to add retrospective listeners to events that have already fired for occasions when you don't know the order in which events will be fired / listeners have been added.

There's an interesting example of this at https://github.com/facebook/emitter#holding

Quick-attach methods.

There should be handy methods to attach hashes to the messaging systems, just like we could do in Wreqr.Radio.

Use one source file?

I'm not entirely sold on the idea myself, but I'll try to make an argument for it since so many Backbone projects do this.

I was just looking at the built file and got thinking that putting everything in one file makes it read rather nicely. Take a minute and read through the ~270 lines of actual code:
https://github.com/jmeas/backbone.radio/blob/master/build/backbone.radio.js

I think that maybe having a bunch of source files makes the library seem larger than it really is, and maybe less approachable.

Our documentation is extensive, however I think a great many people simply read through the source in order to fully understand what the library is doing in the background.

I think it would also reason that we do things that are actually private instead of _private. Such as _debugLog or the upcoming _eventsApi. Two methods that would be weird to override. The reason we don't currently do this is for jshint (which is a weak reason).

For the UMD testing, I think we can probably find another way to keep our coverage at 100%.

@jmeas What do you think?

Reduce repetition in tests

Utilize them wrapper beforeEach and afterEach functions

//cc @thejameskyle

`once` methods are broken

The once methods should pass things off to the on methods, not the trigger methods.

This flew under the radar because there are no tests for this behavior. Code coverage would have helped here :(

Remove the factory function.

The factory function was a neat exercise in merging the two channels, but it is ultimately limiting due to the complexity involved in making unique behavior between Commands and Requests.

At the expense of a larger library I should get rid of the factory and duplicate the code.

package.json bugs.web should be bugs.url

  "bugs": {
-  "web": "https://github.com/jmeas/backbone.radio/issues"
+  "url": "https://github.com/jmeas/backbone.radio/issues"
  },

Command and Request does not support the event handler map syntax

Backbone events supports the, very convenient, event map syntax (allowing me to build my handler mappings in one location and bind them all in another location):

book.on({
  "change:title": titleView.update,
  "destroy": bookView.remove
});

But, the comply and reply methods do not support this. Is this by design?

-

Use one source file?

I'm not entirely sold on the idea myself, but I'll try to make an argument for it since so many Backbone projects do this.

I was just looking at the built file and got thinking that putting everything in one file makes it read rather nicely. Take a minute and read through the ~270 lines of actual code:
https://github.com/jmeas/backbone.radio/blob/master/build/backbone.radio.js

I think that maybe having a bunch of source files makes the library seem larger than it really is, and maybe less approachable.

Our documentation is extensive, however I think a great many people simply read through the source in order to fully understand what the library is doing in the background.

I think it would also reason that we do things that are actually private instead of _private. Such as _debugLog or the upcoming _eventsApi. Two methods that would be weird to override. The reason we don't currently do this is for jshint (which is a weak reason).

For the UMD testing, I think we can probably find another way to keep our coverage at 100%.

@jmeas What do you think?

Extended DEBUG functionality

Throw warnings if you unregister an event that was never registered to catch memleaks.

Alias Backbone.Events as Radio.Events

Should we?

Build to `./tmp` for tests

Yup. This way I don't commit a new built lib each PR.

Test that making a request w. no handler doesn't throw an error

Right now we test that no value is returned, but I want to be complete and test that Errors aren't thrown either

Add tests for `stop` methods when passing a string.

We don't have any :(

Increase coverage to 100%

We can now get the coverage report from coveralls to see the lines that we're missing.

There are 5 lines:

3 from UMD and 2 from noConflict.

This leads to two questions:

1. Can we somehow get rid of the UMD thing and have it only instrument the unbuilt files?
2. Are there clever ways to test noConflict? The only way I can think of doing it is to start a test before Backbone.radio loads...but...is that even possible?

//cc @thejameskyle. I'd love to hear if you have any thoughts on this.

Response => Reply

Response should be reply

Top-level Api should call methods on channel not on Radio

https://github.com/jmeas/backbone.radio/blob/master/src/proxy.js#L15

return channel[methodName].apply(this, args); // this === Backbone.Radio
// should be
return channel[methodName].apply(channel, args);

Update isChannel pattern in Requests

You missed one @thejameskyle ;)

Increase coverage to 99% by not testing UMD

We shouldn't test the UMD wrapper as that lowers coverage. Instead, we should pull in each of the files separately for the tests, instead of relying on .tmp/backbone.radio.js.

This will cause them to be instrumented separately w.o. instrumenting the UMD stuff. The only side effect will be a few additional global variables during testing, but I don't see that being an issue.

Make sure test text refer to the correct system

The tests work fine, but the text was a bit messed up in James' test refactor

I dislike the word 'action', as that isn't even a concept in this lib, so I'm thinking it should be commandName for commands and requestName for requests.

TuneIn functionality

A feature request is TuneIn; an ability to get logs of all of the events going in and out of a channel.

Add API docs.

I've got a getting started section, but I need the entire API.

Stop that phantomJS stuff

According to @thejameskyle if the tests are switched to running in a node env we will get major speed boosts. Worth looking into.

Better name for Commands.react

The current API is:

Events

trigger
on, once, off
listenTo, listenToOnce, stopListening

Commands

command
react, reactOnce, stopReacting

Requests

request
respond, respondOnce, stopResponding

@thejameskyle pointed out that 'react' is not a good name for Commands' handler. I agree. What's better? Hmmm...

Add Gitter Channel to Readme

Doooo it. https://gitter.im/jmeas/backbone.radio

connectX should be the handlers

connectRequests => connectResponses
connectCommands => connectReactions

Test for a Channel with no name passed.

It should throw an error.

Publish to Bower/npm

Indeed

stopComplying and stopReplying should match against callback and context

In Backbone.Events, you can write:

evented.off('foo', myCallback, myContext);
// removes only 'foo' events that have
// `{ callback: myCallback, context: myContext }`

We should add this functionality.

Add Travis

Yuuuup

expose tuneIn _log function somewhere so it can be customized.

https://github.com/jmeas/backbone.radio/blob/master/src/tune-in.js#L9-L12

Fix error thrown when no handlers are registered

If a requests/commands obj has never had any handlers set, and you try to unregister a specific request, it will throw a fatal error.

Yikes.

Add documentation for default handlers

Add docs

Yup. Adding them to the README is fine for now.

Add code coverage / coveralls

@thejameskyle @ahumphreys87 maybe you two could provide some guidance on this?

Stop saying console.errors are being "thrown"

Cause they ain't being thrown.

https://github.com/jmeas/backbone.radio/blob/master/test/spec/debug.js#L13-L17
https://github.com/jmeas/backbone.radio/blob/master/src/requests.js#L20-L23

Top-level API.

You should have access to the top-level API by specifying the channel name, just like Wreqr.radio.

e.g.;

Backbone.Radio.stopListening('global');

Space-separated multiple request/command names

In Backbone.Events, you can write:

evented.on('foo bar', function() {
  // responds to 'foo' and 'bar'
});

We should add this functionality.

Debug mode

Debug mode would log to the console if you call a request or command without any handlers.

Add chart for event cycle for each system

The chart showing how information is transferred between the emitter and the listener is immensely useful. It should be added here.

A generic handler for unhandled commands and requests?

I'm currently working with an application that should proxy commands and requests to the server, when they are not handled by the client.

Is there a way to specifically handle only unhandled commands and requests, similar to a filtered Backbone.on('all', ...);, so that I can bridge my local Backbone.Radio bus and Socket.IO?

Add context tests

You can specify the context of callbacks, but there are no tests for that behavior.

Add test ensuring methods called on one channel don't affect others

There should be a test for this.

beforeEach(function() {
  this.fooStub = this.sinon.stub();
  this.barStub = this.sinon.stub();

  Radio.channel('foo').on('baz', this.fooStub);
  Radio.channel('bar').on('baz', this.barStub);

  Radio.channel('foo').trigger('baz');
});

it('should not call events on other channels.', function() {
  expect(barStub).not.to.have.been.called;
});

Lint the unit tests

It's lawless in there rite now

Consider removing isChannel pattern

var isChannel = this.channelName ? true : false;

if (isChannel) {
  // lala
}

Unnecessary code? We should check if the alternative compiles down to a smaller lib

if (this.channelName) {
  // lala
}

Should channelName be private?

https://github.com/jmeas/backbone.radio/blob/master/src/channel.js#L10

Looking at channels, I'm not sure if channelName should be a public or private property. I figured it warranted a discussion.

Merge spec/radio.js into spec/channel.js

These tests belong in the channel tests.

Get rid of foo/foobar references in the tests

Small thing, but I rly don't like foo/foobar.