[PoC] Use Express-style path matching

[taion]

This demonstrates ripping out all of our path matching logic and just using the logic from Express.

This would be a breaking change due to the change to syntax for optional parameters, along with handling for unnamed parameters (e.g. splats).

This would allow using custom regexes (and potentially path arrays for multiple-choice matching) for path parameters, but I haven't added tests demonstrating that yet.

For more details, for how this would work, see:

• http://expressjs.com/en/guide/routing.html
• https://github.com/pillarjs/path-to-regexp#readme

[taion]

cc @j

[taion]

Thoughts? This matches Express and Koa and seems to hit a good balance of easy and flexible.

It's not my first choice, but it's non-breaking for most use cases (e.g. for all of my current ones), and brings us in line with backend frameworks.

[taion]

I've added a commit with new test cases demonstrating the new possibilities.

[timdorr]

I thought we wanted to make pattern matching pluggable? We would need something like that to maintain a BC path. And then you could have something like <Router matcher={expressMatcher}> for now and put in some deprecation warnings when using the old matcher to inform folks about the change.

As it is, I do like this as a default. It's more expressive and matches an existing paradigm. It looks like the only change is the loss of a non-greedy splat. But we added that fairly recently and only for some edge cases, so I don't think it's a big deal. And they should be able to get around the issue with this new format anyways.

I'm curious if there are implications for how this might change our interaction with Express/Koa. If we have compatible matching rules, we could potentially build an express router to do early matching before you get into a middleware function. You could get some good perf benefits by avoiding much of the common setup code for things like Redux stores and memory histories, and short circuiting for things like 404s and redirect routes. Maybe there are other things you could do too.

[taion]

I was thinking that, but now I sorta feel like – why bother? Do Express or Hapi lose real expressiveness for not having pluggable route matching? Most web frameworks on the backend that I'm aware of just give you a single way to declare routing rules. Seems to work well enough for those in practice.

[timdorr]

I suppose if someone has simple routes and a need for speed, they could write something like fastMatcher that only does simple string matching. Not that I think that's a practical need (the regex compilation is cached and Crankshaft/IonMonkey/FTL should optimize something that hot pretty early on).

Or maybe they want to have an i18n mapping for things like /en-US/programs and /en-GB/programmes.

I don't think those are important and should be something we build for. Just throwing out some ideas. I'm OK with not making it pluggable. But how should we handle BC?

[timdorr]

I might leave this comment block in place, updated to the new syntax. Either that or link to the path-to-regexp docs.

[timdorr]

What about a toggle to the old matcher for BC?

[taion]

That'd definitely be an option. The trick is getting access to the appropriate formatPattern in <Redirect>.

[timdorr]

this.context.router.formatPattern?

[taion]

Not quite – it gets called from https://github.com/reactjs/react-router/blob/v2.0.0/modules/Redirect.js#L38. Probably the best thing might be to set this there to the transition manager.

[taion]

Easier to do a global toggle, I think.

[taion]

My current thought is to have a global toggle, unless we're really confident that having pluggable matching at a global level is worth having.

[timdorr]

I don't think we need to go pluggable. Forget I brought that up :) A toggle makes sense to me. Router.useOldPatternMatching() should work for those that need BC.

[j]

Hey @taion, I brushed through some of this and it looks like it'll fix all the current cases that I have. And this matching code is so much nicer to look at. 😄

[j]

I just brought this in and the only thing that broke right away was optional params. But it was an easy fix. Just change (/:paramName) to /:paramName?

[ryanflorence]

I've wanted to use path-to-regexp for a while (https://github.com/ryanflorence/nested-router/blob/master/index.js)

[taion]

@ryanflorence Do you know why we didn't use it in the first place? Was it just not available at the time or something?

[ryanflorence]

For BC I think we add the "matcher" prop, and then we can warn that the default is changing when they don't provide a matcher in 2.1, then in 3.0 we remove the old matcher, set this as the default

[taion]

@ryanflorence That part is tricky, because there are a number of places where we access PatternUtils as a global singleton, most notably in <Redirect>. We'd have to more or less build full-fledged pluggable matcher support to get this to work. Since path-to-regexp allows users to specify arbitrary regexes for parameters or other parts of their routes, I think we might not actually need pluggable route matching as a feature.

[ryanflorence]

I think main reason for me was I hate the idea of regex route paths, I
expect people to accidentally do weird things with it that they could
easily do with a normal looking route config, but I'm over it.
On Thu, Feb 25, 2016 at 6:38 PM Jimmy Jia [email protected] wrote:

@ryanflorence https://github.com/ryanflorence That part is tricky,
because there are a number of places where we access PatternUtils as a
global singleton, most notably in . We'd have to more or less
build full-fledged pluggable matcher support to get this to work. Since
path-to-regexp allows users to specify arbitrary regexes for parameters
or other parts of their routes, I think we might not actually need
pluggable route matching as a feature.

—
Reply to this email directly or view it on GitHub
#3105 (comment)
.

[ryanflorence]

I'm probably being naive, but I hope the matcher prop is a function that
gets a route and a location and then returns true or false, then we pass it
to PatternUtils etc.
On Thu, Feb 25, 2016 at 6:46 PM Ryan Florence [email protected] wrote:

I think main reason for me was I hate the idea of regex route paths, I
expect people to accidentally do weird things with it that they could
easily do with a normal looking route config, but I'm over it.
On Thu, Feb 25, 2016 at 6:38 PM Jimmy Jia [email protected]
wrote:

@ryanflorence https://github.com/ryanflorence That part is tricky,
because there are a number of places where we access PatternUtils as a
global singleton, most notably in . We'd have to more or less
build full-fledged pluggable matcher support to get this to work. Since
path-to-regexp allows users to specify arbitrary regexes for parameters
or other parts of their routes, I think we might not actually need
pluggable route matching as a feature.

—
Reply to this email directly or view it on GitHub
#3105 (comment)
.

[taion]

@ryanflorence I think the way to do that might be something like @j's #3098, where there's another check after we finish the standard route matching, to let the user say "no match". Perhaps this might even be possible to do with onEnter (as replace(null) or something).

[j]

@taion @ryanflorence yeah, granted I slapped that PR together fairly quickly to get my hands dirty in this code, it's pretty dumb simple. At some point I think every router library needs some sort of "post-match" style functionality where user's can do simple checks without regex (if they want to), and perhaps param/query type casting (preferably supporting async like my example). Keeping it pluggable can allow for people to bring their own stuff. One other example that comes to mind when using a simple "validate" property without any regexes in the path is that I could not even use the match function on the server and dynamically create server-side routes (for the sake of argument, lets say Hapi routes), that way I can take advantage of per-route Hapi caching and not have just one catch all route that renders react views then somehow just call "validate" on the specific route. But this is just getting more complicated, lol.

Anyway, back to what I did, if it returns a "no match", then it moves on to the next route in the chain. Using "onEnter" would sound kind of confusing to allow for moving on? Dunno.

[taion]

onEnter actually wouldn't work, now that I think about it.

Are we all agreed that this sounds like a reasonable way forward? If we're okay with this, plus having a global toggle to switch to path-to-regexp for paths, I can work toward getting this ready for 2.1 or whatever our next point release might be.

[timdorr]

👍 from me.

[j]

That would be awesome. Till then "react-router": "[email protected]:taion/react-router.git#path-to-regexp" for us over here ;)

[taion]

@j Consider moving to your own fork – I'm likely to update this branch soon to make the new matching opt-in, and will probably delete it anyway if it gets merged.

[mjackson]

What is the use case you're trying to solve with this change, @taion? Is it just "let people use regexps"? Or is there some kind of pattern that you can't actually match right now?

People have brought up this issue several times before and it usually just comes down to "i wanna use a regexp". Thing is, by splurging and giving everyone that kind of flexibility, it makes it harder for us to analyze all the paths that could possibly be matched, which could also make it harder to do stuff like warn when a route can't actually be matched.

[GreenGremlin]

If that's the case, why not just document that by using regex routes you (may) lose the following...

[j]

@mjackson shouldn't cases such as the one you mentioned be covered in your tests? Also, your use-case is is more-so checking equality on a route and warning on repeated routes. For regex routes, I'd always add unit tests on the matcher and/or integration tests on the route & handler. And perhaps if warnings are needed, to run known non-regex routes against the regex routes and warn if the regex route comes before the non-regex one?

[taion]

I think the biggest benefit is just not maintaining our own route/regex building code and moving to a style that's fully consistent with Express, instead of something that is close but not quite the same.

This is in fact useful for me for asserting that some routes only take integers or UUIDs as URL params, though, and @j has an interesting use case with enums.

In the fully general case, for testing route reachability, I think the user has to specify values for URL parameters anyway – we can't in general arbitrarily find "safe" URL parameter values (and IMO we shouldn't anyway), so I think in fullness, #2987 will shape up to some sort of utility function anyway.

[mjackson]

Why is everyone so eager to shoot themselves in the foot? Regexps make your route config harder to read! Someone please show me something you can't do with our current path matching.

[taion]

For my use case, I have a number of routes where the only valid value for the URL parameter is an integer. It's pretty convenient to allow this to fall out in the route matching and just drop to the next route. I don't plan on using more complicated regexes than that (and I'm not sure if it would even work).

I expect that most users won't use regexes – the main benefit for the median user is just that we'd have consistent route definition syntax with Express and Koa.

[j]

@taion i agree with the not maintaining the route matching logic.

@mjackson i agree with you as well.

Switching to this matcher does solve my issues, yes. I'd still rather not use regex and instead use a simple matcher function for my enum use-case, but the fact that this PR already will allow me to remove about 20+ hard-coded routes is a win IMO. Also, you don't have to use regex if you don't want. ;)

I still would like to see some sort of matcher function (i.e. #3098) where you don't "need" to use regex for simple cases with built in param conversion (i.e. string numbers to real numbers, string mongo object ids to an ObjectID object, etc, etc). Perhaps the function can be passed in the params and query object and the callback can pass in new param and query objects if they wanted with converted items. That way someone could use something like https://github.com/hapijs/joi if they wanted (which handles conversions for you and returns the new converted object).

For example:

// validations.js
var user = Joi.object().keys({
  params: Joi.object().keys({
    id: Joi.number()
  }),
  query: Joi.object().keys({
    showFriends: Joi.boolean(),
    someEnumField: Joi.any().allow(['a', 'b'])
  })
});

var wrapJoi = (schema) => {
  return (request, callback) => {
    schema.validate(request, (err, value) => {
      if (err) {
        callback(err.isJoi ? null : err, false);
      } else {
        callback(null, true, value);
      }
    });
  };
};

exports.user = wrap(user);


// routes.js
const validations = require('./validations');

const routes = (
  <Route path="/" component={App}>
    <IndexRoute component={Home} />
    <Route path="/users/:id" component={User} condition={validations.user} />
  </Route>
);

The above is "sudo-coded", but I'd be able to share my validation objects that I already have in Hapi with react-router. And you can see that using any validation method could be used. Note the isMatch property can be used to return false to tell the router to move to the next route, or true to try rendering the route, and someone (i suppose) can modify the params to pass an "error" object so that the Component can render appropriate error messages.

[timdorr]

@j I think handling this via a separate prop would be more interesting if that prop was polymorphic. It could take in a function, a regex to apply to all params, or an object of regexes or functions that would get mapped to params of the same name. So, these would all be valid:

// Function(match[, callback])
<Route path="/user/:user_id/posts/:category" condition={someConditionalFunction} />
// Regex
<Route path="/user/:user_id/posts/:category" condition={/\d+/} />
// Object
<Route path="/user/:user_id/posts/:category" condition={{user_id: /\d+/, category: someConditionalFunction}} />

That way you can remain concise and still get the functionality you want here.

[taion]

I don't think we want to do that – having a function that returns true or false is a nice, minimal API, and users can build whatever abstractions they need on top of it.

The reason this change puts the regex in the path isn't because I think it's the right thing to do in some sense – it's because that's what path-to-regexp does, and because I think architecturally it makes sense to use path-to-regexp to handle this, instead of having it in our own code.

[timdorr]

#3098 doesn't add a boolean function prop, but one that deals with a callback. I was referring to doing it in the context of that implementation.

I think architecturally it makes sense to use path-to-regexp to handle this, instead of having it in our own code.

I agree with this. The less we have to do ourselves, the better. The big downside, like @mjackson said, is that this introduces complexity and added difficulty into the "simple" matcher currently used. A new, optional prop is better for developer experience and involves less BC hassle. And it's more powerful.

I'm leaning more towards the extra prop at the moment.

[taion]

How does this introduce complexity?

[taion]

What I mean is:

• This is less code on our side to maintain; we get to rip out our unique bespoke path matching, and it's one less place for bugs to arise
• This is one less route definition system for our users to use – they use the same syntax as they do elsewhere
• The core use case of routes with params-that-match-anything is unchanged

We should think of this as a code-cleaning refactor; that it does bring in support for regex URL params is secondary.

[mjackson]

I'm interested in creating a router that is actually better than stuff
people use elsewhere. To me, the fact that most routers use regexps just
for string matching is a lazy decision that doesn't provide users with much
value, if any at all. Flexibility, sure. But the whole routing system is a
LOT more difficult to understand and it's more difficult for us to build
tooling around it/statically analyze it.

Also, if you guys could please use fewer words that would definitely help
me understand what you're saying. :)

On Wed, Mar 2, 2016 at 12:35 PM Jimmy Jia [email protected] wrote:

What I mean is:

• This is less code on our side to maintain; we get to rip out our
  unique bespoke path matching, and it's one less place for bugs to arise
• This is one less route definition system for our users to use – they
  use the same syntax as they do elsewhere
• The core use case of routes with params-that-match-anything is
  unchanged

We should think of this as a code-cleaning refactor; that it does bring in
support for regex URL params is secondary.

—
Reply to this email directly or view it on GitHub
#3105 (comment)
.

[taion]

To summarize a discussion w/@mjackson on Discord, I think in the near term the plan is to look at doing something like Hapi-style static route conflict detection, which would preclude the use of regexes in routes.

If it's feasible to do that, I think it might be a worthwhile tradeoff. I don't think it'd help for e.g. the case motivating #3098, though.

I think the consensus is to leave this PR on hold for a moment, and see how static checking for route collisions works out.

If anybody here has feedback on how this works in practice with Hapi v. its absence in Express or Koa, I think we'd be very open to feedback on whether such features are useful.

[taion]

This is the specific blocker: #1923 (comment).

[mjackson]

I'd suggest we close this for now since it's something I believe we should not incorporate into the router at this point. I'd be open to re-opening and discussing again later if we get a ways down the road and find scenarios we just can't satisfy with our current route config, but I haven't seen any yet.

[taion]

I think this is useful as a PoC for integrating with an external path-matching library. It's the most straightforward way to get customizable route parameters per #2286, and is going to be familiar to users of Express, Rails, Nginx, Django, &c. This PR isn't mergeable as-is anyway; at this point it's just to demonstrate what would be involved code-wise.

We should figure out on the unreachable routes issue (#1923) what we want to do there.

[timdorr]

@taion Can you push the branch to the @reactjs repo? We can keep it around for possible revisitation in the future.

[taion]

https://github.com/reactjs/react-router/tree/path-to-regexp

My opinion remains that unless we want to go with option 2 per #1923 (comment), our presumption should be in favor of using the most common widely-adopted third-party library for doing our route matching.