Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove commands #221

Merged
merged 1 commit into from
May 22, 2015
Merged

Conversation

paulfalgout
Copy link
Member

Resolves #186

Edited to remove unresolved issue

@jamesplease
Copy link
Member

Thanks @paulfalgout! This doesn't quite resolve #187, though; there are still a few additional changes I'd want to make. No reason to hold back this PR, but if you could change the OP so that Github doesn't autoclose that issue that would be dope.

@jamiebuilds
Copy link
Member

I'll explain the reasoning of this for posterity.

Commands don't have a notable benefit over Requests. In fact it is almost identical code with only one difference: Commands have no return value.

The problem with this comes from testing. When you want to do something asynchronous in a command, you have virtually no way of testing it that doesn't tie itself to the implementation. As where in Requests you can simply return a promise and have everything work nicely.

We discussed adding in message queuing a la Backbone.Wreqr Commands. However there still wasn't a very good use case here over Requests and you run into the issue where you would likely rather know that a command has not yet been registered.

For example, say I command('foo') but my application is buggy and the "foo" command never gets registered. Now my command just sits there forever, creating a memory leak as well as likely difficult issue to debug. It'd be better to know right away that I haven't registered the command.

Hopefully this explains just about everything. I haven't seen many people use commands so I doubt this will be an issue with anyone. We're simply trying to narrow things down to the very best API.

@@ -74,62 +72,19 @@ myBus.trigger('some:event');
```

This is the first principle of Backbone.Radio: building a message bus out of Backbone.Events is useful. But before we go more
into that, let's look at the two other messaging systems of Backbone.Radio.
into that, let's look at another messaging system of Backbone.Radio.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other

This explicitly repeats the fact that there are exactly two, rather than n.

jamesplease added a commit that referenced this pull request May 22, 2015
@jamesplease jamesplease merged commit 3f66771 into marionettejs:major May 22, 2015
This was referenced Jun 26, 2015
@jamiebuilds jamiebuilds mentioned this pull request Jun 29, 2015
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants