-
Notifications
You must be signed in to change notification settings - Fork 322
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
[RFC] Extensible routing in Tide #271
Comments
Use path-tree instead of route-recognizer? |
Just my thoughts, but the solution of least resistance it seems to me is to somehow get the maintainers of route-recognizer to accept http-rs/route-recognizer#21 (along with the tests) and update Tide to use that version with that particular routing behavior, and to keep the router as a core component of Tide for now. I think that's fine, and that would actually likely be my first approach. But given that route-recognizer hasn't been updated in a long time (more than a year), and that its one of the few core dependencies of Tide where documentation is pretty much non-existent, it kind of drives me away from wanting to use it. An ideal router would also have named routes as per #24 in addition to fulfilling the goals mentioned in the RFC. In regards to implementation and usage on Tide, it would seem to me at least intuitively that we key routes on their URI first, instead of the HTTP method, as it would simplify implementing #51 among other things. With that said I'm probably most likely in favor of using https://github.com/rustasync/path-table in Tide, as its a fairly neat looking routing library that fulfills most of these goals (in addition to being a crate under the rustasync umbrella). It just needs a little bit of maintenance, minor/non-breaking in regards to style and documentation. I haven't taken a look at path-tree, actually, but if there's a good demonstration of its' benefits that would be neat 👍 In regards to whether routing should be a middleware or a core component of Tide (as mentioned in #258 (comment)), I have to say I definitely would like to keep it in EDIT: changed some stuff |
Mentioned it in the Discord team channel, and I might as well mention it here too -- I'm in the middle of working on my implementation of the above idea on this branch https://github.com/fairingrey/tide/tree/path_table Note that routes are declared differently in this implementation than in Tide's current form:
This also follows the suggestion at #51 (comment). If a request uses a method that is not allowed at a particular resource (e.g. trying to POST at a resource where only GET behavior exists), instead of returning a 404 it will return a 405. I'm in the middle of trying to figure out how to work out returning automatic OPTIONS endpoints (such that #51 can close), but running into a bit of a snag. The concerning code is here: https://github.com/fairingrey/tide/blob/path_table/tide-core/src/router.rs#L46-L68 Anyways, hopefully that'll give us a good idea on where to go -- I actually don't mind path-table, I think its' general design has been pretty good. Thanks to @yoshuawuyts for helping create it! |
To be fair: quoting @aturon from #141 (comment), route-recognizer is used in crates.io as a transitive dependency ( We've originally used |
Thoughts:
Example
A tree struct from path-tree, just show
How to match?Recursive lookup, follow the steps below:
How to generate URL?let s = custom_format!("/users/{user_id}/repos/{id}/{any}*".trim_end_matches('*'), user_id = 233, id = 377, any = "issues/610");
println!(s); Performancehttps://travis-ci.org/trek-rs/path-tree/jobs/535980099#L564-L601 |
As @yoshuawuyts requested - I've rewritten this as PR #274 to following the template for RFCs that has been provided. This'll allow for more focused discussion around elements of the RFC and provides methods multiple threads of discussion. So I'll close this issue and we can move discussion over there (note I've also fleshed out the RFC more as well, including discussing possible ways of addressing extensibility of routing) |
Summary
This RFC tries to determine if extensible routing is needed in Tide
Motivation
Discussions around PRs #254 (largely on discord) and #258 addressed the question of being able to replace or change the implementation of routing within Tide to address different needs for different applications. The existing routing within Tide has issues that need addressing (#12) and how these issues are resolved tie into the question of extensible routing.
Detailed Explanation
I was initially going to look to address this as a two part question - does Tide need extensible routing, and if so what form should it take. But this actually makes it a large scope and will lead to distraction from the fundamental question - does Tide need extensible routing, or does having a good, understandable and configurable core routing implementation meet the requirements.
Looking back at the the original design goals (see the Digging Deeper section) we have the following goals:
With the further clarification:
The existing routing uses route-recognizer which doesn't appear to be maintained and has some implementation issues that need addressing (Route (metadata) ordering is incorrect.) to provide a more robust routing implementation.
This is integrated into the core of Tide, although WIP in PR #258 did extract this out into a separate module.
Other open issues that relate to routing include:
Discussion points
So with an aim of trying to guide some discussion around this issue I've opened this RFC. If you have previously discussed this issue please reiterate your comments here so we can centralize the discussion.
Some questions that might be helpful for the discussion:
The text was updated successfully, but these errors were encountered: