packages
packages copied to clipboard
[go_router_builder] Add ShellRoute support to go_router_builder
Continuing the work of https://github.com/flutter/packages/pull/3269 Fixes https://github.com/flutter/flutter/issues/111909
Pre-launch Checklist
- [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
- [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
- [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use
dart format
.) - [x] I signed the CLA.
- [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g.
[shared_preferences]
- [x] I listed at least one issue that this PR fixes in the description above.
- [x] I updated
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes. - [x] I updated
CHANGELOG.md
to add a description of the change, following repository CHANGELOG style. - [x] I updated/added relevant documentation (doc comments with
///
). - [x] I added new tests to check the change I am making, or this PR is test-exempt.
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).
If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?
Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.
As mentioned in the other PR, please let me know how you want me to refactor the GoRouterShellGenerator
to avoid duplicated code and I'll write a test based on it.
Cheers.
Yes, it will be better if we can refactor GoRouterGenerator to prevent code duplication. I imagine we will add more shellroute subclass in the future
I've refactored the generator in 76b9b81, let me know what you think.
For the tests, I'm not sure what is the most pertinent. It seems most of the code is already covered by packages/go_router_builder/test/builder_test.dart
or packages/go_router_builder/example/test/ensure_build_test.dart
. Let me know what use case you want to test.
Thanks!
hi @GP4cK , Thank you very much for your solution. There may be a little problem.
If I add another route
, the generated file has two List<RouteBase> get $appRoutes => [...
For example, I add LoginRoute
to the bottom of shell_route_example.dart
:
///ignore above
/// Login route
@TypedGoRoute<LoginRoute>(path: '/login')
class LoginRoute extends GoRouteData {
const LoginRoute();
@override
Widget build(BuildContext context, GoRouterState state) =>
LoginScreen();
}
class LoginScreen extends StatelessWidget {
const LoginScreen({super.key});
@override
Widget build(BuildContext context) {
return const Text('Login');
}
}
The shell_route_example.g.dart
has two $appRoutes
:
/// ignore other code
List<RouteBase> get $appRoutes => [
$myShellRouteData,
];
/// ignore
List<RouteBase> get $appRoutes => [
$loginRoute,
];
The correct $appRoutes
should be:
List<RouteBase> get $appRoutes => [
$myShellRouteData,
$loginRoute,
];
The reason may be hear:
There are two GoRouterGenerator
in go_router_builder.dart
。
Builder goRouterBuilder(BuilderOptions options) => SharedPartBuilder(
const <Generator>[
GoRouterGenerator(
annotation: 'TypedGoRoute',
routeClass: 'GoRouteData',
),
GoRouterGenerator(
annotation: 'TypedShellRoute',
routeClass: 'ShellRouteData',
),
],
'go_router',
);
but the method generate
of GoRouterGenerator
returns:
@override
FutureOr<String> generate(LibraryReader library, BuildStep buildStep) async {
/// ignore
return <String>[
'''
List<RouteBase> get \$appRoutes => [
${getters.map((String e) => "$e,").join('\n')}
];
''',
...values,
].join('\n\n');
}
@kuyacai thanks a lot for this.
I've refactored the generator so that it generates only a single $appRoutes
in 7f4c6e0
ok, thx!
Sorry if this PR is not the best place to comment on this, but I've been trying to use the @TypedShellRoute
in my code, which appears in go_router
, but seems to be ignored by go_router_builder
. It almost seems like the bits to make this work are part of the go_router
package, but the go_router_builder
package has not been updated yet to actually generate them.
Is that a fair assessment of where this is at, at the moment?
Is that a fair assessment of where this is at, at the moment?
That's correct. Right now I'm waiting for @chunhtai or @johnpryan 's advice on how to manage the navigatorKey
and the parentNavigatorKey
.
We cannot pass them to TypedShellRoute
/ TypedGoRoute
because GlobalKey<NavigatorState>()
is not a const.
So either we can either:
- declare them as a static member called
$navigatorKey
in the typed route and the generator will pick them up (but it's not obvious for users of this library) - use the workaround described in https://github.com/flutter/flutter/issues/119294#issuecomment-1474867983 that clones each routes after the code generation to add the keys before passing them to GoRouter.
Ah okay. I hope it gets sorted soon because it's quite confusing at the moment to have TypedShellRoute
available in go_router
, but for it to have no effect when build_runner
runs. Thanks for the update @GP4cK
Hi @GP4cK can you help me understand why it needs to be const? I couldn't find the place the enforce it
I just learned from @johnpryan that annotation has to be const. Let me think a bit on what else we can do here...
Yes, annotations must have a const
constructor, so all of the fields must be final and all of the parameters must be const too. Since Key
s in Flutter aren't const, I'm not sure how we're going to support navigatorKey
or parentNavigatorKey
.
Is that a fair assessment of where this is at, at the moment?
That's correct. Right now I'm waiting for @chunhtai or @johnpryan 's advice on how to manage the
navigatorKey
and theparentNavigatorKey
.We cannot pass them to
TypedShellRoute
/TypedGoRoute
becauseGlobalKey<NavigatorState>()
is not a const. So either we can either:
- declare them as a static member called
$navigatorKey
in the typed route and the generator will pick them up (but it's not obvious for users of this library)- use the workaround described in [go_router_builder] Add parentNavigatorKey to generated routes. flutter#119294 (comment) that clones each routes after the code generation to add the keys before passing them to GoRouter.
I will give a try to this PR since I built around everything with Typed
. Thank you for the workaround.
@GP4cK
After some thought, I think we can have a getter in the GoRouteData and ShellRouteData
for example:
abstract class GoRouteData extends RouteData {
...
GlobalKey<NavigatorKey> get $parentNavigatorKey => null;
...
}
abstract class ShellRouteData extends RouteData {
...
GlobalKey<NavigatorKey> get $navigatorKey => null;
...
}
and user can override and provide their navigator key in the subclass if they want. I was hoping this would be more clear than using the static field. At least people can see the getter in the abstract class and notice such thing exists.
WDYT @johnpryan @GP4cK
I'm good with that and it's not too far from the initial implementation.
EDIT: actually the problem is that if the keys are non-static getters, I don't know how to invoke the getter or get the name of the variable returned by it in the generated code.
For example, in this code:
final GlobalKey<NavigatorState> goRouteKey = GlobalKey<NavigatorState>();
class GoRouteWithKey extends GoRouteData {
const GoRouteWithKey();
@override
GlobalKey<NavigatorState>? get navigatorKey => goRouteKey;
}
In the _decodeKey()
method, we can find the navigatorKey
with
classElement.fields
.where(whereKeyName)
.where((FieldElement element) {
final DartType type = element.type;
if (type is! ParameterizedType) {
return false;
}
final List<DartType> typeArguments = type.typeArguments;
if (typeArguments.length != 1) {
return false;
}
final DartType typeArgument = typeArguments.single;
if (typeArgument.getDisplayString(withNullability: false) ==
'NavigatorState') {
return true;
}
return false;
})
But once we have the ref to the navigatorKey
, how to know that the return value is goRouteKey
?
I have a question regarding @TypedShellRoute
. Let's say I have two screens: SplashView
& HomeView
. The first screen shown is SplashView
, generated by @TypedGoRouter
, after the native splash screen. After SplashView
, it will directly transition to HomeView
which is generated by @TypedShellGoRouter
.
Apparently, to run this flow, I should put HomeView
as a childRoute
element. If I do not, it does not generate the code.
Code
final GlobalKey<NavigatorState> _navigatorKey = GlobalKey<NavigatorState>();
final GlobalKey<NavigatorState> _shellNavigatorKey = GlobalKey<NavigatorState>();
final router = GoRouter(
navigatorKey: _navigatorKey,
initialLocation: AppRoutes.SPLASH,
observers: [NavigatorObservers()],
routes: $appRoutes,
);
/// Splash Route
@immutable
@TypedGoRoute<SplashViewRoute>(path: AppRoutes.SPLASH, routes: [])
class SplashViewRoute extends GoRouteData {
const SplashViewRoute();
@override
Widget build(BuildContext _, GoRouterState state) => const SplashView();
}
///
/// Home Route
@TypedShellRoute(
routes: [
TypedGoRoute<SubViewRoute>(path: AppRoutes.HOME),
],
)
class HomeViewRoute extends ShellRouteData {
const HomeViewRoute();
@override
Widget builder(BuildContext context, GoRouterState state, Widget navigator) => const SizedBox.shrink();
@override
GlobalKey<NavigatorState>? get navigatorKey => _shellNavigatorKey;
}
class SubViewRoute extends GoRouteData {
const SubViewRoute();
@override
Widget build(BuildContext _, GoRouterState state) => const SizedBox.shrink();
@override
GlobalKey<NavigatorState>? get navigatorKey => _shellNavigatorKey;
}
If I turned HomeView
to @TypedGoRoute
, I can navigate without declaring it as a childRoute
.
Am I doing something wrong? Should not I at least navigate to SubView
?
I am getting this error when I tried to generate a code:
The @TypedGoRoute annotation must have a type parameter that matches the annotated element.
package:routes/routes.dart:38:7
╷
38 │ class HomeViewRoute extends ShellRouteData
edit: pubspec.yaml
environment:
sdk: ">=2.19.5 <3.0.0"
flutter: 3.7.9
dependencies:
flutter:
sdk: flutter
cupertino_icons: ^1.0.2
# Routing
go_router: 6.5.2
dev_dependencies:
flutter_test:
sdk: flutter
flutter_lints: ^2.0.0
# Code Generation
analyzer: 5.10.0
build_runner: 2.3.3
build_verify: 3.1.0
# Builders
# TODO: do not forget to change this to upstream when its merged
go_router_builder:
git:
url: [email protected]:GP4cK/packages.git
path: packages/go_router_builder
ref: feature/shell-route-go-router-builder
flutter:
uses-material-design: true
@GP4cK @chunhtai
I have a question [...]
@kaankoken, I copy/pasted your code in the example, ran the generator and it did generate some code. The only line I modified was
/// Home Route
-@TypedShellRoute(
+@TypedShellRoute<HomeViewRoute>(
Also, I see that you're overriding the navigatorKey
but with the current state of this PR, this will do nothing. You should replace it like this:
-@override
-GlobalKey<NavigatorState>? get navigatorKey => _shellNavigatorKey;
+static final GlobalKey<NavigatorState> $navigatorKey = _shellNavigatorKey;
yeah I was confused myself how GoRouteData.$route
was called. it looks like using static field may be cleanest way to approach this. Sorry for all the detours, let's go with the original proposal.