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

More powerful/flexible path matching #142

Closed
mjackson opened this issue Jul 29, 2014 · 71 comments
Closed

More powerful/flexible path matching #142

mjackson opened this issue Jul 29, 2014 · 71 comments
Labels
Milestone

Comments

@mjackson
Copy link
Member

Let's make string-based pattern matching more flexible to accommodate some of the use cases we're starting to see.

In particular, I'd like to propose we follow Sinatra's lead here and:

  • Allow ? to mean "match the previous thing 0 or 1 time"
  • Modify * to not match . since it can be excluded using ?

By just doing these two changes, we can support the following kinds of URLs:

  • /posts/:slug.?:format? (make the .html portion of a URL optional)
  • /files/*.* (get the file extension in a separate splat)
  • /archive/? (optional trailing slash)

I've already got this functionality in mach and it's working really well. I rarely (read: haven't yet needed to) use a regular expression to match on a URL path. I realize that this discussion will probably evolve to include supporting full-blown regular expressions, but I'd like to discuss that in a separate issue if people are interested.

@bobeagan
Copy link
Contributor

Wouldn't #58 cause your first example to not work as expected? Seems like it make the whole :slug. param optional or did I misunderstand that change?

@mjackson
Copy link
Member Author

@bobeagan Yes, this change would make #58 not work anymore. It's what I was working toward originally when * didn't match . but I didn't get ? implemented in time.

@ryanflorence
Copy link
Member

I'm totally 👍 on all of this.

What about path="*" though?

I have one very strong request to maintain and equivalent to what <Route path="*"/> means today. Primary use-case is a NotFound route, but there are certainly others.

Right now you can do this:

<Routes>
  <Route handler={App}>
    <!-- routes -->
  </Route>
  <Route path="*" handler={NotFound}/>
</Routes>

This lets you easily handle routes that aren't found, gives you a chance to redirect when you've changed routes, etc. etc. (#139 (comment))

Why not just use a <Routes defaultHandler={NotFound}/>

We've talked about having a defaultHandler that matches when nothing else does. Implementing that would make it so we don't need an equivalent to the current path="*". My guff with that is that it breaks the nesting paradigm to share UI with the NotFound handler and the other views.

If I have a logged in user, and some route is not found, I still want my application chrome up in the top of the browser--or not, I should be able to do either.

If we simply come up with an upgrade path for path="*" then we can continue to do things like this:

<Routes>
  <Route handler={App}>
    <!-- routes -->
    <!-- NotFound now shares the App UI chrome :D -->
    <Route path="*" handler={NotFound}/>
  </Route>
</Routes>

Proposed upgrade paths for path="*"

<Route path="..."/>
<Route path="**"/>
<Route path={Router.NOT_FOUND}/>
<NotFoundRoute/>
<Route path="*"/> <!-- special case naked *, if no other characters are present -->

@ryanflorence
Copy link
Member

doesn't sinatra match anything with *?

@mjackson
Copy link
Member Author

From the Sinatra README:

get '/say/*/to/*' do
  # matches /say/hello/to/world
  params[:splat] # => ["hello", "world"]
end

get '/download/*.*' do
  # matches /download/path/to/file.xml
  params[:splat] # => ["path/to/file", "xml"]
end

The * in Sinatra matches anything up to a ., ?, or /. The implementation details are here.

@ryanflorence
Copy link
Member

but doesn't just * mean "match anything else?

http://stackoverflow.com/questions/10806024/how-can-i-give-sinatra-a-catchall-default-route

@mjackson
Copy link
Member Author

Not AFAIK. That SO question was answered by the same guy who asked it in the first place. I doubt the answer is accurate.

@sophiebits
Copy link
Contributor

(FWIW answering questions you've asked is explicitly endorsed by SO.)

@ryanflorence
Copy link
Member

okay, well, regardless of what sinatra does, thoughts on supporting the current path="*" use-case with some new API?

@mjackson
Copy link
Member Author

My vote would be to use ... to mean "whatever else is left". It's going to be used in ES6, as well as in a few places in React (I've heard).

Only problem is I guarantee someone is going to try /files/.../name.jpg, which won't work...

Hey, but that actually doesn't look too bad. Maybe instead of altering the behavior of * we just introduce ... which basically means "match anything up to the next ?, /, or .". Hmm...

Hmmm... but the problem with that is that now

/files/*.*

becomes

/files/.......

...

@mjackson
Copy link
Member Author

I guess we could just error if someone tries to use anything after ...

@ryanflorence
Copy link
Member

I'm actually laughing out loud about /files/.......

@ryanflorence
Copy link
Member

yeah, ... is not a delimiter, its a symbol for catchall. Only restriction is that nobody can have the url .... I'm not sure I want to support that person's use-case anyway.

@ryanflorence
Copy link
Member

I kindof like <NotFoundRoute/>. There's nothing to learn or explain. You start your first day at a job and see:

<NotFoundRoute handler={NotFound} />
<!-- v. -->
<Route path="..." handler={NotFound} />

With the second, there's a chance you scratch your head at ..., even though you'll probably understand whats happening, you'll wonder if you understand how. With the first, there isn't even a potential question.

@mjackson
Copy link
Member Author

Wait a minute ... what if you just leave off the path entirely?

We currently use this pattern in root routes to mean "this route matches no matter what, check the children to see if it's currently active". We had a big discussion about it.

We could expand on that definition to say "if I don't have any children, then I'm the route you're looking for". This is entirely consistent with the way we currently treat the absence of path.

In summary: no path === always match

BTW, this also removes the need for <Routes defaultHandler/>.

<Routes>
  <Route name="home" handler={Home}/>
  <Route name="about" handler={About}/>
  <Route handler={NotFound}/>
</Routes>

@ryanflorence
Copy link
Member

What about when it's nested inside home?

@mjackson
Copy link
Member Author

What about when it's nested inside home?

In that case it would match when the URL is /home and none of home's other children matched.

Honestly this makes more sense if we use relative paths instead of absolute, which I've been thinking about switching to anyway.

@mjackson
Copy link
Member Author

If nested paths build on those of their parents it's a little easier to understand:

<Routes>
  <Route name="users" handler={Users}>
    <Route name="user" path=":id" handler={User}/>
    <Route handler={UsersNotFound}/> <!-- matches /users/unknown-path -->
  </Route>
  <Route name="posts" handler={Posts}>
    <Route name="post" path=":id" handler={Post}/>
    <Route handler={PostsNotFound}/> <!-- matches /posts/unknown-path -->
  </Route>
  <Route handler={SiteWideNotFound}/> <!-- matches /unknown-path -->
</Routes>

Edit: Just realized that example doesn't make a ton of sense because /posts/:id already matches /posts/unknown-path...

@bobeagan
Copy link
Contributor

👍 to @mjackson's suggestion

@bobeagan
Copy link
Contributor

Constraints on the params might help with that /posts/:id problem... at least partially

@ryanflorence
Copy link
Member

I'm okay with nesting if

  • paths that start with / are absolute (like today)
  • everybody agrees we never nest the names too (good grief I don't like that in ember)

However ... The route config is not about url nesting to me, its about view nesting. It seems nearly every person that I've helped bootstrap a new ember app starts nesting routes in order to nest urls, and then get all sorts of confused about why their views are nesting. I really dislike this coupling.

@ryanflorence
Copy link
Member

<Routes>
  <Route name="users" handler={Users}>
    <Route name="user" path="user/:id" handler={User}/>
    <Route name="notFoundUser" path="user/*" handler={UsersNotFound}/> <!-- matches /users/unknown-path -->
  </Route>
  <Route name="posts" handler={Posts}>
    <Route name="post" path="post/:id" handler={Post}/>
    <Route name="notFoundPost" path="post/*" handler={PostsNotFound}/> <!-- matches /posts/unknown-path -->
  </Route>
  <Route path="*" handler={SiteWideNotFound}/> <!-- matches /unknown-path -->
</Routes>

That's what it looks like with the current API.

@ryanflorence
Copy link
Member

Wait, in both of our examples you'd never get to those not found routes because with post/unknown, this.props.params.id becomes unknown, and now we're into @bobeagan's comment about constraints.

@mjackson
Copy link
Member Author

The route config is not about url nesting to me, its about view nesting.

Yes, strongly agree. The emphasis should always be on UI nesting, and requiring absolute paths helps people to understand that better IMO. So let's leave the paths absolute.

I do still like the config I mentioned earlier and I think it makes a lot of sense when there is only one level of routes. But it falls down when we start using path-less nested routes, so the only real use case for a route that always matches is in the root of your config (assuming absolute paths). Everywhere else it's just a trap.

So, AFAICT it seems like the only real use case for <Route path="*"> today is for not found, c/d?

@mjackson
Copy link
Member Author

If so, let's just follow up in #140 and be done with it...

@ryanflorence
Copy link
Member

But how do I nest that in my global app chrome?

@JedWatson
Copy link
Contributor

For what it's worth it would be great if the route matching syntax more or less matched that used by express.

I'm working on a concept for how to share both routing logic and React components between the client and server, where the server would be node.js with express and the client would use react-router.

So if they're the same, the definitions can potentially be shared, or at least you don't have to transpose between the two formats.

It looks pretty close now (express supports regular expressions but maybe that's not worth also matching) and express is very similar to Sinatra so maybe this isn't a big thing.

Also, this may be of interest if you haven't seen it: http://visionmedia.github.io/page.js/

@ryanflorence
Copy link
Member

@JedWatson yeah, I've been thinking about this also. Being a clone or subset of express's routes is probably the best route since most server-side rendering is going to happen in node, and therefore probably express.

However, when we have all of our AsyncState pieces built, you just hand the url from node to the router, you don't actually need server routes because your react app will handle it.

And yes, I was using page.js before I wrote this router :)

@simenbrekken
Copy link
Contributor

@rpflorence Either the way I'm doing it now with routes that take a pattern parameter:

<Routes>
  <Route pattern={categoryPattern} handler={Category} />
  <Route pattern={productPattern} handler={Product} />
</Routes>

This however leads to a horrible mess when it comes to <Link /> et al. I'd much rather like to be able to create my own route type and just do the matching manually:

var RegExpRoute = React.createClass({
    mixins: [RouteMixin],

    findMatches: function(path, query) {
        var matches = this.props.pattern.match(path);

        if (matches) {
            return {
                params: {
                    category: matches[1],
                    product: matches[2],
                    variant: matches[3]
                }
            };
        }
    },

    makePath: function(params) {
        // Build a path using the extracted params
    }
});

@couchand
Copy link

@rpflorence I quite like the simplicity of the results in #142 (comment). I think I understand the distinction between DefaultRoute and NotFoundRoute but it's probably worth making it explicit.

For the (simplified) example:

<Routes location="history">
  <Route handler={App}>
    <DefaultRoute handler={Home}/>

    <Route name="course" path="/courses/:courseId" handler={Course}>
      <DefaultRoute handler={CourseDashboard}/>

      <NotFoundRoute handler={CourseNotFound}/>
    </Route>

    <NotFoundRoute handler={NotFound}/>
  </Route>
</Routes>

We get the following routes:

Absolute Path Handler
(nothing) App > Home
/ App > Home
/courses App > NotFound
/courses/ App > NotFound
/courses/123 App > Course > CourseDashboard
/courses/123/ App > Course > CourseDashboard
/courses/123/something-else App > Course > CourseNotFound
/anything-else App > NotFound

With the possible exception that /courses/123/ might actually resolve to CourseNotFound, depending on your preference for trailing slashes. (My opinion on that is to allow them by default, but I guess that's a separate issue).

@ryanflorence
Copy link
Member

@couchand you got it!

@dazld
Copy link

dazld commented Aug 12, 2014

If we were to want to internationalise those routes (ie, translate them /en/courses/ -> /pt/cursos/ how would you approach this, in the structure above?

@ryanflorence
Copy link
Member

Awesome question, there are a few approaches off the top of my head, but I think I'd do something like this:

<Routes>
  <Route handler={App}>
    <Route name="course" path={localizePath('/courses/:id')} handler={Course} />
  </Route>
</Route>
var paths = {
  en: 'courses/:id',
  es: 'curso/:id'
};
function localizePath() {
  return paths[userLocale];
}

It would require a window.reload() when the user changes locales, but I think that's fine.

@dazld
Copy link

dazld commented Aug 12, 2014

looks good @rpflorence - edit: ah, actually, not sure that would work for multiple paths as written. but, can see the technique. the fn call would be everywhere though, which smells like something that could be refactored. I don't want to derail this thread, but wanted to raise it as a topic to be considered. could open another issue to talk about that separately.

@ryanflorence
Copy link
Member

Another option is a bunch of redirects based on locale in the "primary" language route. But yeah, if you'd like, start a gist and we can continue this conversation.

@mjackson
Copy link
Member Author

I appreciate the discussion about <NotFoundRoute> that we've had here. Seems like we're all in agreement about how it should work. Let's follow up any further discussion about it in #140.

Now, to get back to path matching, does it make sense to proceed with the modifications I originally suggested?

@rpflorence Seems like your concern about path="*" is solved using <NotFoundRoute>, c/d?

mjackson added a commit that referenced this issue Aug 19, 2014
[changed] * in paths no longer matches .
[added] Support for arrays in query strings

Fixes #142
@brigand brigand mentioned this issue Aug 22, 2014
mjackson added a commit that referenced this issue Aug 27, 2014
[changed] :param no longer matches .
[added] Support for arrays in query strings

Fixes #142
@skratchdot
Copy link
Contributor

@mjackson - why was support for decimal places in params removed (in commit: d5bd656)? Are we not allowed to have params that are numbers now (i.e. floating point numbers)?

@mjackson
Copy link
Member Author

@skratchdot You can no longer match a . with a :namedParam, but you could still easily match a floating point number using something like /my/:number.:decimal

@skratchdot
Copy link
Contributor

@mjackson - Thanks for the explanation. That's not going to work in my use-case. I need my param to be almost any string (specifically relating to colors). For example: red, 33FF00, rgba(23,45,200,0.9), hsl(239.65, 67.06, 50), etc

IMHO a param should be able to be any character besides these 4:

  • ?
  • &
  • /

I might be forgetting a few, but it's late!

Anyways, I have a workaround, but it would be nice if I could "fix" this. Will a pull request be accepted if I get it to work, or is there a philosophical reason for not allowing decimals in params?

@mjackson
Copy link
Member Author

@skratchdot If you need to match . you should use *.

"/colors/*" // matches /colors/red, /colors/33FF00, /colors/rgba(23,45,200,0.9), etc.

Also, it's important to note that * is not greedy. This means that you can put more path after it if you like.

"/colors/*/edit" // matches /colors/red/edit, /colors/rgba(23,45,200,0.9)/edit, etc.

You can access the segment of the URL that was matched by a * using this.props.params.splat.

@skratchdot
Copy link
Contributor

@mjackson - great! thank you for taking the time to explain that. i should now be able to remove the hack / workaround I had in place.

@ryanflorence
Copy link
Member

#260 makes . match just fine now.

@mjackson
Copy link
Member Author

oh man, i totally forgot about that. have we cut a release with that in it?

@davidhq
Copy link

davidhq commented Jul 14, 2015

is <Routes ignoreTrailingSlash={true}> supposed to work in 1.0 ? Doesn't seem so :/ What is the optimal way to achieve optional trailing slashes? /? also doesn't work...

@command-tab
Copy link

Is ignoreTrailingSlash on by default? Seems to me like that would be the most sane default.

@davidhq
Copy link

davidhq commented Jul 14, 2015

@commandtab doesn't look like it is, but it would be great!

@sassanh sassanh mentioned this issue Aug 7, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.