-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
More powerful/flexible path matching #142
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
Comments
Wouldn't #58 cause your first example to not work as expected? Seems like it make the whole |
I'm totally 👍 on all of this. What about
|
doesn't sinatra match anything with |
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 |
but doesn't just http://stackoverflow.com/questions/10806024/how-can-i-give-sinatra-a-catchall-default-route |
Not AFAIK. That SO question was answered by the same guy who asked it in the first place. I doubt the answer is accurate. |
(FWIW answering questions you've asked is explicitly endorsed by SO.) |
okay, well, regardless of what sinatra does, thoughts on supporting the current |
My vote would be to use Only problem is I guarantee someone is going to try Hey, but that actually doesn't look too bad. Maybe instead of altering the behavior of Hmmm... but the problem with that is that now
becomes
... |
I guess we could just error if someone tries to use anything after |
I'm actually laughing out loud about |
yeah, |
I kindof like <NotFoundRoute handler={NotFound} />
<!-- v. -->
<Route path="..." handler={NotFound} /> With the second, there's a chance you scratch your head at |
Wait a minute ... what if you just leave off the 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 In summary: no path === always match BTW, this also removes the need for <Routes>
<Route name="home" handler={Home}/>
<Route name="about" handler={About}/>
<Route handler={NotFound}/>
</Routes> |
What about when it's nested inside home? |
In that case it would match when the URL is Honestly this makes more sense if we use relative paths instead of absolute, which I've been thinking about switching to anyway. |
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 |
👍 to @mjackson's suggestion |
Constraints on the params might help with that |
I'm okay with nesting if
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. |
<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. |
Wait, in both of our examples you'd never get to those not found routes because with |
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 So, AFAICT it seems like the only real use case for |
If so, let's just follow up in #140 and be done with it... |
But how do I nest that in my global app chrome? |
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/ |
@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 And yes, I was using page.js before I wrote this router :) |
@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 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
}
}); |
@rpflorence I quite like the simplicity of the results in #142 (comment). I think I understand the distinction between 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:
With the possible exception that |
@couchand you got it! |
If we were to want to internationalise those routes (ie, translate them |
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 |
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. |
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. |
I appreciate the discussion about 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 |
[changed] * in paths no longer matches . [added] Support for arrays in query strings Fixes #142
[changed] :param no longer matches . [added] Support for arrays in query strings Fixes #142
@skratchdot You can no longer match a . with a :namedParam, but you could still easily match a floating point number using something like |
@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? |
@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 |
@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. |
#260 makes |
oh man, i totally forgot about that. have we cut a release with that in it? |
is |
Is |
@commandtab doesn't look like it is, but it would be great! |
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:
?
to mean "match the previous thing 0 or 1 time"*
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 separatesplat
)/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.
The text was updated successfully, but these errors were encountered: