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 all 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
65 changes: 60 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(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.
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] = {
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,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
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.

};
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;
}

});
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