Added condition property to do extra tests on route params, etc.

[j]

This is sort of a WIP of what could be extremely useful for routing. IMO, there should be a few other methods of doing what I have here, such as a a "requirements" field to do simple regex without the need of creating a function as well as a "defaults" to set param defaults.

I've ran into the need to have more of an ability to match multiple routes and do some sort of check. I've resorted to insane hacks to get around it.

This PR shows just the "condition" method being used.

function isValidCategory(match, callback) {
  callback(null, ['baking', 'bbq'].indexOf(match.params.category) > -1)
}

const routes = (
  <Route path="/" component={App}>
    <IndexRoute component={Home} />
    <Route path=":category(/:page)" component={Category} condition={isValidCategory} />
    <Route path=":slug" component={Page} />
  </Route>
);

I haven't dug too much into the code, I just wanted to see how hard it would be to add this functionality, and it turns out that the way I did it didn't take more than 10 minutes to do.

[taion]

We actually discussed an API in #2286, and built some consensus over the desirability there. Did you check that issue?

[j]

@taion yes, I've seen a ton of discussions about similar things and in Discord, people ask the same question (or similar questions that could be solved by better route matching/validation). I didn't really chime in on what everyone was suggesting, but I don't see why having just a specific matcher for a route makes sense instead of just doing param validation and upon not validating, it can move on to the next route. Which is what this method does. I initially just had a "requirement" parameter that took in regex, but this one solves most everyone's pains in 15 lines of code. This is just an attempt at showing code instead of just saying, ":+1: yay"

[ryanflorence]

I wish there was a more generalized way to do this, like, instead of baking in a condition prop to routes, it would be nice if the matching had some spot to plug into that would allow you to add a convention like this instead.

[ryanflorence]

<Router match={someMatchingThingThatUsesDefaultWithExtraStuff}/>

[taion]

@j brought up on Reactiflux the concern with #2286 that the proposed generalized deserialization from URL parameters would make the current formatPattern very clunky to use – something like this might actually be our best bet.

[ryanflorence]

import { matchRoute } from 'react-router'

function matchWithCondition(location, params, route) {
  return route.condition ? route.condition(params) : matchRoute(...arguments)
}

<Router matchRoute={matchWithCondition}/>

or something...

[taion]

And we can't just drop formatPattern because we need it for <Redirect>.

[ryanflorence]

we have middleware-ish ways to enhance histories, and middleware-ish ways to do rendering RouterContext, the last bit of code we still have all wrapped up in a black box is matching.

[taion]

What specifically are we trying to accomplish?

If we just need a way to restrict the value of a URL parameter to some set of values, and give users a nice way to fall through to the NotFound route, then this is actually pretty good. The route component itself can handle deserialization, and we can call it a day from the POV of the router.

[taion]

Right now we support doing e.g.

<Route path="/users/:userId">
  <Redirect from=":postId" to="/posts/:userId/:postId" />
</Route>

I'm concerned about this breaking with arbitrary matcher functions.

[taion]

I wonder if we could just use path-to-regexp and call it a day.

[timdorr]

The pattern matcher should function like a middleware, both determining what URLs match a pattern and how the params come out the other side. Say you have some SEO-friendly URL structure (I have this in one of my apps) like /item/1234-foo-bar-baz. You might have a path='/item/:itemid', but you only want the numeric part. The pattern matcher could be customized to remove any non-numeric parts.

[timdorr]

And just to give some inspiration, ui-router in the Angular world supports custom matching via rule() on its provider: https://github.com/angular-ui/ui-router/wiki/URL-Routing#rule-for-custom-url-handling

Definitely don't want that, but maybe it will spawn some ideas. shrug

[taion]

That breaks formatPattern though.

[timdorr]

I think any pattern matching middleware thing should be able to both serialize and deserialize the URL to/from a location descriptor and params.

[taion]

Deserialization can be a separate concern from matching. It can even be in user space.

[timdorr]

I feel like those belong together. It would be like useQueries not implementing createHref.

[taion]

useQueries is a fine example, I think – it deserializes the query string into an object, but doesn't deserialize those values from strings into business values.

It seems like "do what Express does" is a pretty good baseline, and it would let us handle e.g. this use case with a very minimal change.

[taion]

Given that we're stalled on #3105, I think we should try to move forward here.

I like the idea of having an async hook that lets us set up a route to not match. One of the biggest missing features in React Router is that we don't set up any good way to 404 on e.g. https://github.com/reactjs/non-existent-repo, and this looks like the best way to accomplish that.

[taion]

I'd factor out the callback to getIndexRoute into a variable in this block, and just do something like:

if (!route.condition) {
  getIndexRoute(route, location, finishMatch)
}

Or perhaps a better name instead of finishMatch.

[taion]

If I'm not mistaken, condition only gets evaluated on the last route in the match. This is probably not what we want – it should run on every route in the branch.

[j]

@taion I fixed the last route issue and added tests for it. Still need to add more complex tests and get that .2% coverage back.

I did include a slight refactoring in the match logic to split up the matchRouteDeep method. One thing I noticed after taking your advice on where to put it is that the condition function gets ran in a backwards order when the route should fail match should fail as soon as the parent's condition isn't met. b2dc1d5#diff-7f51f7b8bcc1ab598f60a81f4bc21724R429 <- this line was failing.

A few more things I want to do or discuss is what params should be passed to each condition function. I was thinking of all that are known until that point will be passed. Also, should more things other than params be passed in? Browser/etc. Or have a configurable context for each "request".

/cc @timdorr @ryanflorence

[j]

Also, one thing I'm not fond of at the moment are that with a lot of conditions on routes, it can potentially lead to performance issues from the high amount of createParam calls. Well, in general, it seems that a large amount of requests/route calls will have a performance hit (??? I'm assuming at the moment) due to the PatternUtils#matchPattern function returning two arrays that keep getting merged over and over upon match. I think we can help that by passing the param object to PatternUtils#matchPattern and where the match.slice(1).map line is, append new keys and values as they come and use that in place of the createParams calls.

[taion]

I wouldn't worry about performance for now – since this is an internal API, we can always change stuff as needed. I also don't think we can do much to optimize – we can't mutate params objects in place. I wouldn't be too worried about the cost of building a few more objects and arrays.

[kevinsimper]

@taion Coming from #3210. Could react-router access a function inside the component? This way the logic could be inside the component.

class App extends Component {
   validateRouting() {
    // react-router calls this function to determine if it is okay to route
    return 404;
  }
  render() {
    // normal render function
  }
}

[taion]

If you declare a static method on the component, there's no way we can prevent you from using that.

That said, that will not be the preferred API – it's too inflexible and doesn't e.g. work nicely with Flux contexts or other integrations. That's why we've moved away from static component methods in the first place.

[ryanflorence]

I'm 👍 on this, just need to resolve the conflicts.

[taion]

I'm going to push this back to 2.4.0 – I think we need a 2.3.0 release to "fix" a few minor semver quibbles.

[taion]

@j Are you still working on this? I might take it over if not.

[j]

Hey @taion, yeah go ahead and take over. Currently swamped at work.

[taion]

Same, unfortunately. I'll leave this open until I have time to take a closer look.

[jamesplease]

I like the callback API for validation. One slight downside is that it seems like a lot of boilerplate to do simple validation, like requiring one param to be numeric. But it would work great for more complex validation.

If alternative APIs for this is still being considered, another idea could be one inspired by ng-router. I haven't seen this brought up in any issues (I'm probably just missing it, though). In ng-router, they provide a syntax in the DSL to override the regex used by dynamic segments. Here's an example, which doesn't use ng-router's particular syntax, but accomplishes the same goal:

books/:bookId=[0-9]{1,4}

This would only capture numeric bookIds with between 1 and 4 characters. ng-router provides a few "keywords" out of the box, too, which provide the most common alternatives, such as numeric. i.e.;

books/:bookId=numeric

The pros to this approach is that the validation is defined "inline" in the route definition itself, rather than requiring a callback. So it could be more succinct, which I found is great for simple validation. But the cons are that it could get messy for more complex validation. Also, it might be considered a con that users must write RegEx – and that they'd be writing it in a string, embedded in some other DSL. Is that doubly gross?

One possibility which slightly mitigates the issue of complex matching would be to allow users to "register" custom RegEx's which they could then use in the route definition. I've considered adding this to my own router, but I never did because I wasn't a huge fan of the API.

In my experience, simpler validation is the 99% use case. But there are a few times when I've needed more complex things.

Anyway, I think the callback API is good if it's decided. Getting this feature out there will be a big win for this lib ✌️

[taion]

The point of this PR is to offer a syntax to e.g. hit up a remote to see if /user/:userId should 404.

We've considered using Express's syntax for regex routes #3105 and probably want to move in a direction of allowing configuring the matching engine on a route-by-route basis.

[jamesplease]

The point of this PR is to offer a syntax to e.g. hit up a remote to see if /user/:userId should 404.

Right, right. I got that.

We've considered using Express's syntax for regex routes #3105 and probably want to move in a direction of allowing configuring the matching engine on a route-by-route basis.

Oh, nice. I didn't even see that PR! I'm a big fan of that option, too. Is there any consensus around which approach might be merged? I could even see an argument that both make sense. A callback for complex stuff, and custom RegExp for simple stuff.

[taion]

It makes it hard to do Hapi-style static validation of routes if we always allow regexes for paths.

I think the path forward is to allow route-by-route configuration. Then we can set things up such that e.g. if you only use a simple path specification syntax, that we can statically validate routes – if not, well, you don't get that, but you can use regex params.

This is obviously a lot more work.

[taion]

I'm closing this out for now given the changes in master that make this not mergeable as-is.

I personally need something like this feature eventually to e.g. dynamically 404 on objects not existing on the backend, but it's a little tricky.