Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

add generic route event #2062

Closed
wants to merge 3 commits into from

7 participants

@bodokaiser

Hello,

I often got the use case that I want to inform some object (e.g. nav view) on route changes.

Unfortunately currently it is only possible to inform that object on specific route changes even I often just want to get informed about any route change and get the triggered route as parameter.

To implement this I just added one line code which now triggers on the router a "route" event with the first parameter being the route name and the other parameters being the arguments we would have at a "route:example" event.

Regards,
Bodo

@braddunbar
Collaborator

Hi @bodokaiser! Thanks for the patch. Is there any reason you can't use Backbone.history.on('route', ...) for this situation?

@bodokaiser

Backbone.history would basically work the same but I actually find it semantically much better to access the router for this all routing stuff and it would fit good to the route:route events.

Also it maybe could be problematic accessing history when it is maye unavailable (because of its state as global).

@akre54
Collaborator

:+1: with @bodokaiser on this. There's a lot of times where the route event is specific to a particular Router and would work better than listening to and filtering every route event passed to Backbone.History.

@braddunbar
Collaborator

This seems like a fine idea to me, but I'm not sure that this is the api we want. Since the arguments are variable (unlike "route:name" events) we should probably pass them as an array similar to the Backbone.history event, right?

@akre54
Collaborator

@braddunbar yes, absolutely. I'd vote for the same method signature. So you can listen to a specific route off the Router, all routes off the Router, or all routes off History in a straightforward way.

@WillsB3

+1 for a global route event. I've worked around it in the past by binding to the "all" event on the router in my routers initialize function, and then searching the passed "event" name parameter for "route:" and if it matches then triggering the blanket "route" event on the router myself. Of course this is pretty much a hack and it would be great to have this functionality as part of Backbone. Using Backbone.history.on is cumbersome, since, as was previously mentioned, it's global.

I might be going mad here, but I'm certain that I've seen this exact same patch submitted before. I remember seeing the discussion I'm sure. I'll have a hunt for that issue and let you know if I find it.

@WillsB3

As I remember, It's been mentioned a few times, See #419, #650, #701 (and @jashkenas patch 5b43cd9).

I'm not sure if/how that affects things, but just thought I'd let you know about the possible previous discussion on the subject.

@bodokaiser

Is this discussion going in to the direction of defining a totally new event api for the router?

I think that would be too much. Adding a generic route event would match the already used events api for example of the model (change, change:attribute).

@braddunbar
Collaborator

@bodokaiser No, I don't think so. I was just pointing out that the "route" event should probably get the route parameters as an array instead of extra arguments since they will vary with different routes.

@bodokaiser

@braddunbar should I update the pull request with providing the parameters as extra arguments or should I wait for more discussion?

@braddunbar
Collaborator
@bodokaiser

ah github updates the request automatically. Would be worth a notice ^^

@braddunbar commit is ready

backbone.js
@@ -963,6 +963,7 @@
Backbone.history.route(route, _.bind(function(fragment) {
var args = this._extractParameters(route, fragment);
callback && callback.apply(this, args);
+ this.trigger.call(this, 'route', name, args);
@tgriesser Collaborator

This can just be this.trigger('route', name, args);, apply is only used on the next line to make the trigger call with an array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bodokaiser

changed trigger call to @tgriesser suggestion.

@braddunbar
Collaborator

Thanks @bodokaiser! Merged, added test, and swapped the order to match the rest of Backbone's more to less specific ordering (first "change:attr", then "change").

@braddunbar braddunbar closed this
@vincentbriglia

@braddunbar : I am trying to understand why this was added, since Backbone.Router extends from Backbone.Events, why isn't the all event enough here? Is it solely for returning the arguments in array form? isn't that just easily solved by slicing all but the first argument into an array [].slice.call(arguments, 1);. Perhaps I am missing something, but this seems like it could easily be achieved like this:

router.on('all', function (r) {
    console.log(r, [].slice.call(arguments, 1));
});
// output: route:help ["troubleshooting"] 

router.on('route:help', function () {
    console.log([].slice.call(arguments, 0));
});
// output: ["troubleshooting"]

router.navigate('help/troubleshooting', {trigger: true});

adding an additional event seems redundant, but can you please -for my sanity- confirm either way?

@braddunbar
Collaborator

Hi @vincentbriglia! I'd be glad to explain. You may want to check out my comments in #2087 as well.

You're right that this could be handled via listening to "all".

myRouter.on('all', function(event) {
  var match = event.match(/^route:(.*)$/);
  if (!match) return;
  var name = match[1];
  var args = [].slice.call(arguments, 1);
  // ...
});

However, that's a good amount of boilerplate that could be much more easily written as

myRouter.on('route', function(name, args) {
  // ...
});

Also, I think the symmetry between "route"/"route:name" and "change"/"change:attr" is nice and promotes the convention. Does that clear it up at all or does the event still seem redundant?

@vincentbriglia

I see, I always forget that you can arbitrarily assign and trigger events that aren't necessarily Backbone's own on any object that extends from Backbone.Events. In case of the router, that's why you're checking for the route match. That indeed adds a bit more boilerplate than I initially thought necessary, so that's gained some argumentative weight. :+1: I have no quarrel with this particular change anymore

@braddunbar
Collaborator

Great! Glad I could clarify. :)

@WillsB3

@braddunbar I've done exactly what you quote to work around the lack of a generic "route" event in a large scale app running on 0.9.2. Really glad to see the generic route event is on the way. I also think that the parity between "route" / "route:name" and "change" / "change:attr" is good (Infact, I often use this style to namespace my Backbone events when I am likely to have many being triggered on one object)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 2, 2013
  1. added trigger applyment

    bodokaiser authored
Commits on Jan 4, 2013
  1. route parameters provided as array argument

    bodokaiser authored
Commits on Jan 7, 2013
  1. changed trigger call

    bodokaiser authored
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 0 deletions.
  1. +1 −0  backbone.js
View
1  backbone.js
@@ -963,6 +963,7 @@
Backbone.history.route(route, _.bind(function(fragment) {
var args = this._extractParameters(route, fragment);
callback && callback.apply(this, args);
+ this.trigger('route', name, args);
this.trigger.apply(this, ['route:' + name].concat(args));
Backbone.history.trigger('route', this, name, args);
}, this));
Something went wrong with that request. Please try again.