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

Router add use object params option #3815

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 59 additions & 5 deletions backbone.js
Original file line number Diff line number Diff line change
Expand Up @@ -1455,6 +1455,16 @@

// Set up all inheritable **Backbone.Router** properties and methods.
_.extend(Router.prototype, Events, {
// If set to true, route handlers will be called with routeData object
//
// this.route('search/:query/p:num', 'search', function(routeData) {
Copy link
Author

Choose a reason for hiding this comment

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

@akre54

I don't know about the routeData object though. How about (params, queryString, name) as the arguments to the handler? Or (params, routeData) where routeData is queryString and name.

I think I like (params, routeData) more. Having routeData as object gives option to expand the object with more stuff to help the route handler if needed, few things I could think of:

  • query - object representation of queryString: 'x=true&y=5&z=abc' => {x: "true", y: "5", z: "abc"}.
  • fragment

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 to routeData over 1...n args. I'd like to see Backbone's router give even more info in route callbacks, and a 1...n arg API would get a little unwieldy with that many args.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be ok with (params, routeData). The most important piece of information here is the params object, hiding it inside routeData just adds friction.

Copy link
Author

Choose a reason for hiding this comment

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

👍

// // for route 'search/backbone/p5?a=b'
// // routeData.params.query === 'backbone'
// // routeData.params.num === '5'
// // routeData.queryString === 'a=b'
// });
//
useParamsObject: false,

// Initialize is an empty function by default. Override it with your own
// initialization logic.
Expand All @@ -1467,12 +1477,26 @@
// });
//
route: function(route, name, callback) {
if (!_.isRegExp(route)) route = this._routeToRegExp(route);
// Pass paramNames into _routeToRegExp and let that function populate it
var paramNames = [];
// Need to know this so we don't try to create params object when the route is matched
var isRegExpRoute = _.isRegExp(route);

if (!isRegExpRoute) route = this._routeToRegExp(route, paramNames);

if (_.isFunction(name)) {
callback = name;
name = '';
}
if (!callback) callback = this[name];

this._routeInfos = this._routeInfos || {};
this._routeInfos[route.toString()] = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

#toString shouldn't be necessary, it'll be coerced by the object.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I will update. Thanks.

name: name,
isRegExpRoute: isRegExpRoute,
paramNames: paramNames
};

var router = this;
Backbone.history.route(route, function(fragment) {
var args = router._extractParameters(route, fragment);
Expand Down Expand Up @@ -1511,13 +1535,18 @@

// Convert a route string into a regular expression, suitable for matching
// against the current location hash.
_routeToRegExp: function(route) {
_routeToRegExp: function(route, paramNames) {
Copy link
Author

Choose a reason for hiding this comment

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

@akre54

I'm not so keen on modifying passed arguments. Let's bikeshed this decision a bit more (you may have been right in the first place).

I don't like this either, but this feels the least intrusive/most optimal since we are already RegExp parsing the input route string. And we can reuse namedParam and splatParam right where they are used.

Since _routeToRegExp() is only used from route(), how about moving it's body inside route() itself? Might be breaking change if someone is ovewritting _routeToRegExp() though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about moving it's body inside route() itself?

I'm concerned about taking a nice small chunk of reusable, functionally pure code and pulling it out of a method and into the body of an already big method.

I don't have a ton of time to work through this at the moment, but I should be freer next week. Try opening a pull and solicit feedback from other Backbone contributors. Looking forward to seeing this merged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what express uses, so passing the array is probably fine.

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking about this too. What if this method, instead of returning RegExp, returns an object {routeRegExp, paramNames}. It would keep it functionally pure.

route = route.replace(escapeRegExp, '\\$&')
.replace(optionalParam, '(?:$1)?')
.replace(namedParam, function(match, optional) {
return optional ? match : '([^/?]+)';
if (optional) return match;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should be recording optional params as well.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, they are being recorded. At this point, this replace handles all named parameters, including optional ones, as previous replace already appends : to the name:

Edit: It handles splash params too. As I have in the test cases.

'named/optional/(y:z)'
  .replace(escapeRegExp, '\\$&')                    // 'named/optional/(y:z)''
  .replace(optionalParam, '(?:$1)?')                // 'named/optional/(?:y:z)?'
  .replace(namedParam, function(match, optional) {
    // this function gets called twice
    // 1st
    //   match: '(?:y'
    //   optional: '(?'
    // 2nd
    //   match: ':z'
    //   optional: undefined
  });

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

paramNames.push(match.slice(1));
return '([^/?]+)';
})
.replace(splatParam, '([^?]*?)');
.replace(splatParam, function(match) {
paramNames.push(match.slice(1));
return '([^?]*?)';
});
return new RegExp('^' + route + '(?:\\?([\\s\\S]*))?$');
},

Expand All @@ -1526,11 +1555,36 @@
// treated as `null` to normalize cross-browser behavior.
_extractParameters: function(route, fragment) {
var params = route.exec(fragment).slice(1);
return _.map(params, function(param, i) {
var decodedParams = _.map(params, function(param, i) {
// Don't decode the search params.
if (i === params.length - 1) return param || null;
return param ? decodeURIComponent(param) : null;
});

if (this.useParamsObject) {
// get routeInfo, which has array of param names
var routeInfo = this._routeInfos[route.toString()];

var routeData = {
name: routeInfo.name
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure the route's name is useful. Is there a usecase?

Copy link
Author

Choose a reason for hiding this comment

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

I am currently using that right now in my app, where I hold on to current route data. And I use route.name to know what route it is.

Something along the line of router.getCurrentRoute, since it's not part of the route handler, name is what tells me what current route is.

};

if (routeInfo.isRegExpRoute) {
// if original route is RegExp, everything goes into .params
routeData.params = decodedParams;
}
else {
routeData.params = _.object(routeInfo.paramNames, decodedParams);
var queryString = decodedParams[decodedParams.length - 1];
if (queryString) {
routeData.queryString = queryString;
}
}

return [routeData];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd much rather have these returned as separate parameters: [paramsObject, queryString], instead of a nested object.

Copy link
Author

Choose a reason for hiding this comment

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

So the handler would receive (paramsObject, queryString) as parameters, right?

We also discussed about this up here:
#3815 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to read better, will continue discussion there.

}

return decodedParams;
}

});
Expand Down
1 change: 1 addition & 0 deletions test/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
<script src="model.js"></script>
<script src="collection.js"></script>
<script src="router.js"></script>
<script src="router-with-params-object.js"></script>
<script src="view.js"></script>
<script src="sync.js"></script>
</body>
Expand Down
Loading