-
Notifications
You must be signed in to change notification settings - Fork 152
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
feat: Support reuse of nested router #393
feat: Support reuse of nested router #393
Conversation
Hey @davidmartos96 👋
Not sure I follow, can you provide a bit more context regarding where excess Routers are dynamically created? In the meantime, I’ll run some benchmarks to gather some more quantitative data. Thanks again! 🙏 UPDATE I ran some preliminary benchmarks using apache benchmark and did not observe any significant performance differences between the latest version of Dart Frog (route with nested, dynamic middleware: Apache Benchmark Results on 2019 MacBook Pro (intel i9)Dart Frog Dev
Dart Frog Prod
Dart Frog Prod Compiled
Shelf
Shelf Compiled
|
So taking the following snippet from the tests as an example: Handler createUserHandler(String user) {
final router = Router()
..get('/self', (RequestContext context) {
return Response(body: "I'm $user");
})
..get('/', (RequestContext context) {
return Response(body: '$user root');
});
return router;
}
final app = Router()
..mount('/users/<user>', (RequestContext context, String user) {
final handler = createUserHandler(user);
return handler(context);
})
..all('/<_|[^]*>', (RequestContext context) {
return Response(body: 'catch-all-handler');
}); Each request to a resource in
So where I see it could be problematic is a situation where you have a nested router ( Another point jonasf makes in the other thread is the following:
Having the creation of the router inside a handler prevents you to early-detect a possible error in a subroute right when you start the server. |
@felangel Hello! From my tests on my machine, with this PR, a dynamic nested router server performs 44% better. |
@felangel Any update on this? It would be great if you could take a look, as this could be beneficial to |
Sorry for the delay! I’m planning to revisit this sometime this week. Thanks again! |
Any updates? 😅 |
Hi @davidmartos96 , sorry for the delayed reply. We've not forgotten about this Pull Request. I will try looking at it, but it might take slightly longer than expected since I need to gather some context first. |
@alestiago Great to hear! The initial discussion around this was in the linked Original issues: Benchmarks: |
Status
PENDING DISCUSSION
Description
Hello! I've just noticed that supporting nested routers has been implemented, great news.
The implementation is very similar to one of my early iterations in dart-lang/shelf#288 . But according to jonasfj, creating a Router per each request could be resource intensive.
What I propose is a way to access the mounted params from within the RequestContext/Request so that a Router can be created before hand.
The implementation I did in this PR is the same solution as in the last iteration in the
shelf_router
PR, but there might be other solutions. I'm open to discuss it. I'm used to work with shelf and shelf_router as is, not with the dart_frog code generation, so there might be a solution that could fit nicely with it.Type of Change