Generated extension methods for goNamed and pushNamed
#66
Comments
|
My plan had been to wait until the static meta programming feature in Dart was available to add typesafe routing but I like the idea to add it optionally now. Do you have a PR to share? |
|
No PR to share yet. I think this would be a significant amount of work, so I'd want to brainstorm it together and get full buy-in before breaking any ground. |
|
Having a generated file that provided for type safe wrappers for named routes would make an excellent addition to go_router. |
|
@craiglabenz and I did some brainstorming and came up w/ the following core abstraction to noddle on: // go_router provides
abstract class MaterialGoRoute {
Widget builder(BuilderContext context, GoRouterState state) =>
MaterialPage(key: state.pageKey, child: buildPage(context, state));
Widget buildPage(BuildContext context, GoRouterState state);
}
// user defines a top-level route
@goroute('home', path: '/')
class HomeRoute extends MaterialGoRoute {
@override
Widget buildPage(BuildContext context, GoRouterState state) => HomePage();
}
// user defines a sub-route (under HomeRoute)
@goroute('family', path: 'family/:fid', parent: HomeRoute)
class FamilyRoute extends MaterialGoRoute {
@goparam('fid') final String fid;
FamilyRoute(this.fid);
@override
Widget buildPage(BuildContext context, GoRouterState state) {
final family = Families.family(fid);
return FamilyPage(family);
}
}
// user defines a sub-route (under FamilyRoute)
@goroute('person', path: 'person/:pid', parent: Family)
class PersonRoute extends MaterialGoRoute {
@goparam('fid') final String fid;
@goparam('pid') final String pid;
PersonRoute(this.fid, this.pid);
@override
Widget buildPage(BuildContext context, GoRouterState state) {
final family = Families.family(fid);
final person = family.person(pid);
return PersonPage(family, person);
}
}
//... codegen magic happens...
// user can call static push/go methods; compiler enforces the right params
HomeRoute.go(context);
FamilyRoute.go(context, fid: family.id);
PersonRoute.go(context, fid: family.id, pid: person.id);
PersonRoute.push(context, fid: family.id, pid: person.id); |
|
cc @kevmoo |
|
Here is my opinion on typed routing and its corresponding documentation. Do bear in mind that I have strong opinions regarding code gen and routing so feel free to ignore my remark and please do not feel offended if I express a different opinion than yours. GoalI love what this is trying to solve. About the docs, the issue is well explained but the example is quite untruthful imo: final selectedAuthor = libraryInstance.allAuthors.firstWhereOrNull((a) => a.id == authorId);Would still be needed even with code gen. I would remove that part and write Route treeI like the fact that all In my opinion:
In any case, this should be mentioned in the docs. Also in the doc I find the order of the route in the example quite confusing at first. Since this is so early in the doc my first read took some effort to see which things were Error page builderIs there a reason why we cannot have NavigationI like the compiler error, this is what this entire thing is built for so that’s great 👍 I’m not sure about the
I guess it doesn’t matter here but it still bugs me in a way. The solution would be to be able to use Query and path parametersOptional parameter = Query Parameters | Required parameters = Path Parameters is very opinionated. I think this is a great default behavior however what if you have an optional parameter with a default value ? Or a required parameter which can be null ? Extra parametersAbout I am quite confused by this but maybe it’s my ignorance about code gen: could we not directly use Also are named extra parameters allowed ( About the concept of extra parameters I won’t hide the fact that I don’t like extra parameters (VRouter bears witness), but this might be a chance to solve the issues it has. Mainly the fact that when navigating with the url to a A solution could be to require them to have a default value, and check that they do during code gen (I don’t know if this is possible). RedirectionNo comment on the API, however the example is wrong I think: // This is what is used
final loggingIn = state.subloc == HomeRoute().location
// But this should be used instead right?
final loggingIn = state.subloc == LoginRoute().locationRoute-level redirectionWait, do you mean that overriding However this is in line with the current API so I understand the choice. I just don’t like it, neither here nor in the original Transition overrideTo change the page key, could we have a Note on the docs if this CustomI would give the child in Page<void> buildPage(BuildContext context, Widget child, GoRouterState state) => FancyPage(
key: state.pageKey,
child: child,
);That’s not vital but it took me some time to realize why Documentation comments, hided because it is quite lenghtyNits: you kept I would replace the example with a fully fledged one. Currently devs:
Add to that the fact that devs are often confused by the Page API so they have a hard time differentiating what is package specific vs what is vanilla Flutter and that gives you a very poor experience. Anyway here is how that could look like: class FancyRoute extends GoRouteData {
@override
Widget build(BuildContext context, GoRouterState state) => FancyScreen();
@override
Page<void> buildPage(BuildContext context, Widget child, GoRouterState state) => FancyPage(
key: state.pageKey,
child: child,
);
}
class FancyPage extends Page {
FancyPage({required this.key, required this.child});
final LocalKey key;
final Widget child;
@override
Route createRoute(BuildContext context) {
return PageRouteBuilder<dynamic>(
pageBuilder: (_, animation, __) => FadeTransition(
opacity: animation,
child: child,
),
);
}
}Naming conventionsI really don’t like the naming conventions. According to me in Flutter there is General remarksI like what it is trying to solve. I don’t like code generation, but I guess that’s better than runtime errors. If macro could solve this too once they land, that would be awesome (but I understand that you don’t want to wait for the dart language to change). On that note, if the long term goal is to move to macros: Are macro mature enough that we could know if this could easily be a 1 to 1 replacement? I think the code gen API should strive to be replaceable by macro without feature change once they arrive. What happens if a Also the signature of |
|
@lulupointu thanks for the great feedback. I left some responses inline.
That's true. The part I meant to highlight is the line of code above it, which is not code you have to write when using codegen: final authorId = int.parse(state.params['authorId']!);
You need to annotate the top-level route, so the route tree needs to be in the same place as that.
Yep. Naming is hard. I'm open to suggestions.
What would it do? How would the codegen know what code to produce?
In a world where Dart allowed for overriding the go() method with different route types, that would certainly be an option.
This is already have go_router works. I don't want the typed routing to change the foundations of go_router; merely provide a boost in robustness and productivity.
I want to be quite clear that $extra does not go through the location, making it worthless for deep and dynamic linking and, if you're going to use it, it's OK for it to be a little weird so that you'll have to think twice about using it. The good news is that if you do use $extra and then change your mind, you can change your route classes and let the compiler guide you to all of the users of those routes for you to change.
Yes.
I don't like them either but it unblocks some scenarios for folks that want to use go_router but don't care about deep and dynamic linking. I want to enable it but I don't want to make it without speed bumps.
Good catch!
I hear you on using more types == letting the compiler be stricter. However, it also requires the user to go to the effort of switching types. By the time they do that, I feel like they could've implemented the redirect method.
In my experience, switching keys is rare; If I can keep the number of concepts down, then that's a win overall. If it becomes a problem, it's easy enough to add over time.
The idea is that you override buildPage or build -- there's no real reason to override both or to have the result of one passed to the other.
That was an earlier draft of the spec. The example code has been updated.
I agree that screen vs. route vs. page vs page stack is poorly understood in Flutter. I made a stab at naming in the existing go_router package and I don't thing changing the terminology for typed usage would be helpful for folks.
That's the ideal for sure.
go() will be missing from any GoRouteData-derived class that isn't part of the @TypedGoRoute() attribute, so it will be a compile-time error.
Can you point those out to me? I looked again through the docs and couldn't find any instances of |
|
I will answer only what makes sense, but thanks for all the other answers as well.
I know, what I am saying is that is would make more sense to change the example to: GoRoute(
path: ':authorId',
pageBuilder: (context, state) {
// require the authorId to be present and be an integer
final authorId = int.parse(state.params['authorId']!);
return MaterialPage<void>(
key: state.pageKey,
child: AuthorDetailsScreen(authorId: authorId),
);
},
),Since this focuses on the part the codegen is trying to solve.
I was not talking about the naming here but rather the organization in the file. Here is what would make more sense to me: // Define your [GoRouteData]s
class HomeRoute extends GoRouteData {...}
class FamilyRoute extends GoRouteData {...}
class PersonRoute extends GoRouteData {...}
class LoginRoute extends GoRouteData {...}
// Bind them to the path using [TypedGoRoute]
@TypedGoRoute<LoginRoute>(path: '/login');
@TypedGoRoute<HomeRoute>(
path: '/',
routes: [
TypedGoRoute<FamilyRoute>(
path: 'family/:fid',
routes: [
TypedGoRoute<PersonRoute>(
path: 'person/:pid',
),
],
),
],
);But that's just me. Maybe ask other what they think (some who have never read it before if possible)
I understand you approach and it makes sense. The only thing I am still unsure: If you use
Agree to disagree 😉 Would you consider
Actually I was not arguing about
I must have been confused with a Also you did not comment on the "Complete transition example" and you did not update the doc in the regard. Is there a reason ? Or maybe you missing my comment because it was hidden in a summary ? |
Ah. Yes. I see. I just dropped in some code I copy/pasted from a sample, but you're right. Your code focuses on the issue better. I'll update the proposal.
Unfortunately I believe that the limitation of codegen annotations is that they have to annotate something.
Yes. That's the reason to avoid the $extra parameter.
I considered that, but it seemed redundant. Without it, I write: GoRouter(redirect: (_) => BooksRoute().location)with it, I write: GoRouter.redirect(redirect: (_) => BooksRoute().location)It doesn't seem to be a win.
Were you suggesting a different name? Perhaps GoPage? GoPageData?
I missed your feedback there. Can you repeat it? |
Are you sure you have read the code correctly ? I just changed the ordering of the declaration but not the code itself. What I wrote is not valid ?
I see. They maybe consider changing the documentation FROM
TO
Or something along those line. The most important part being the impossibility to dynamic/deep linking. The word defeats is to vague and does not convey any meaning in itself
What about: GoRouter.redirect((_) => BooksRoute().location)Exactly the same but you harvest the power of the IDE
I was thinking about a different name but a think Why do I say This is why is confusing about those API as well, but there is not much to be done about it, as I said: If I had any comment it would be about the examples, trying to as list make the naming consistent, this means changing:
Then people do what they want in their code but I think in the documentation this would help new comers keep track of what is what.
Sure: Nits: you kept I would replace the example with a fully fledged one. Currently devs:
Add to that the fact that devs are often confused by the Page API so they have a hard time differentiating what is package specific vs what is vanilla Flutter and that gives you a very poor experience. Anyway here is how that could look like: class FancyRoute extends GoRouteData {
@override
Widget build(BuildContext context, GoRouterState state) => FancyScreen();
@override
Page<void> buildPage(BuildContext context, Widget child, GoRouterState state) => FancyPage(
key: state.pageKey,
child: child,
);
}
class FancyPage extends Page {
FancyPage({required this.key, required this.child});
final LocalKey key;
final Widget child;
@override
Route createRoute(BuildContext context) {
return PageRouteBuilder<dynamic>(
pageBuilder: (_, animation, __) => FadeTransition(
opacity: animation,
child: child,
),
);
}
} |
|
Oh man, please don't go crazy with codegens! The main reason I switched to GoRouter was the lack of them. 😂 I get the value of the above, but if you do decide to add this, please make it optional. And please don't bake codegen into the "main workflow" of the package. |
Before the @TypedGoRoute was annotating the top-level route. With your proposed reorganization, if I'm reading it correctly, that's no longer the case and the annotations don't actually annotate anything.
I'll take that into account when/if this proposal ever turns into real docs for a real implementation.
I like syntax completion and letting the compiler tell me I did something wrong, but I'm sure this particular juice is worth the squeeze. For example, sometimes I want a conditional redirect and a pageBuilder. In that case, I have to eschew the GoRoute.redirect() ctor and choose the main GoRoute() ctor. That feels confusing to me.
That's too much typing, I think. I really like how
I was relying on Dart covariance (contravariance?) and the fact that markdown is a very forgiving compilation environment. Once I have working samples and can turn this proposal into actual docs, I'll fix things like that.
I don't need to do all of that in my existing untyped routing example for custom transitions. Am I missing something in that sample? |
Oh believe me. I get it. The codegen routing solutions I've seen are not worth the complexity of the build_runner, which is why I didn't start there with go_router and why I don't plan on making it the default usage. I'm hoping to leverage Dart macros when those are available, which should help make the codegen solution a lot easier, but we're not there yet.
That's the plan for sure. |
Do you mean that, because there is also some codegen for
Yeah I don't know why your sample is so complex. Isn't |
I'm not talking about formatting. I'm talking about my understanding of Dart annotations which I believe require something to annotate, e.g. // this is OK
@foo
class Something {}
// this is not
class Something {}
@foo@kevmoo is the authority here.
Interesting. I think go_router's support for custom transitions is pretty simple. |
I see, then it's quite counter intuitive, I though I understood the use I suppose the alternative it to add a
Actually I did some intense tests today and your approach is more bulletproof. For reasons explained here. |
Hey. I got lucky. : ) |
|
Reverted in 696f7d7 |
|
@csells I have a question about the approach of type safe routing. Will the way of defining GoRouter change? or the same way when meta programming is released? |
|
@YazeedAlKhalaf – no change is planned for folks doing hand-written routing. There are some new APIs added that are used by generated code so there is less boilerplate for you to write! |
Fixes csells/go_router#66 Adds two new classes to pkg:go_router
This is an exploratory feature request to add a
build_runnerlayer for generating type-safe wrappers aroundgoNamedandpushNamed.I'm imagining something like this:
Which would create something like this:
Thoughts?
The text was updated successfully, but these errors were encountered: