-
-
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 4 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(routeData) { | ||
// // 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. | ||
|
@@ -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()] = { | ||
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.
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. 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); | ||
|
@@ -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) { | ||
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 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 Since 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 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 commentThe 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 commentThe 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 |
||
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,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 | ||
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 |
||
}; | ||
|
||
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]; | ||
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'd much rather have these returned as separate parameters: 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. So the handler would receive We also discussed about this up here: 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 need to read better, will continue discussion there. |
||
} | ||
|
||
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 think I like
(params, routeData)
more. HavingrouteData
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
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.
👍 to
routeData
over1...n
args. I'd like to see Backbone's router give even more info in route callbacks, and a1...n
arg API would get a little unwieldy with that many args.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'd be ok with
(params, routeData)
. The most important piece of information here is theparams
object, hiding it insiderouteData
just adds friction.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.
👍