Skip to content

Conversation

ntgn81
Copy link

@ntgn81 ntgn81 commented Oct 2, 2015

Adding optional parameter to Router to make route callback handlers called with route params being an object instead of array.

#3799

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.

@ntgn81
Copy link
Author

ntgn81 commented Oct 9, 2015

Any backbone guru have time to take a look at this and give me guidance on how to make this better?

Thanks!

backbone.js Outdated
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.

@ntgn81
Copy link
Author

ntgn81 commented Oct 28, 2015

Updated routed handler to receive (params, routeData) with tests.

Removed the route.toString in hash lookup too.

@jridgewell @akre54

@wlepinski
Copy link

Is this PR planned to land on master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants