-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
base: master
Are you sure you want to change the base?
Changes from all commits
aa3b3f5
883dcd9
b26ef94
5baa236
628353d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(params, routeData) { | ||
// // for route 'search/backbone/p5?a=b' | ||
// // params.query === 'backbone' | ||
// // params.num === '5' | ||
// // routeData.queryString === 'a=b' | ||
// }); | ||
// | ||
useParamsObject: false, | ||
|
||
// Initialize is an empty function by default. Override it with your own | ||
// initialization logic. | ||
|
@@ -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] = { | ||
name: name, | ||
isRegExpRoute: isRegExpRoute, | ||
paramNames: paramNames | ||
}; | ||
|
||
var router = this; | ||
Backbone.history.route(route, function(fragment) { | ||
var args = router._extractParameters(route, fragment); | ||
|
@@ -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) { | ||
route = route.replace(escapeRegExp, '\\$&') | ||
.replace(optionalParam, '(?:$1)?') | ||
.replace(namedParam, function(match, optional) { | ||
return optional ? match : '([^/?]+)'; | ||
if (optional) return match; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should be recording optional params as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, they are being recorded. At this point, this 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
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]*))?$'); | ||
}, | ||
|
||
|
@@ -1526,11 +1555,37 @@ | |
// 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]; | ||
|
||
var routeData = { | ||
name: routeInfo.name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
}; | ||
var routeParams; | ||
|
||
if (routeInfo.isRegExpRoute) { | ||
// if original route is RegExp, everything goes into .params | ||
routeParams = decodedParams; | ||
} | ||
else { | ||
routeParams = _.object(routeInfo.paramNames, decodedParams); | ||
var queryString = decodedParams[decodedParams.length - 1]; | ||
if (queryString) { | ||
routeData.queryString = queryString; | ||
} | ||
} | ||
|
||
return [routeParams, routeData]; | ||
} | ||
|
||
return decodedParams; | ||
} | ||
|
||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akre54
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
andsplatParam
right where they are used.Since
_routeToRegExp()
is only used fromroute()
, how about moving it's body insideroute()
itself? Might be breaking change if someone is ovewritting_routeToRegExp()
though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.