[go_router_builder] Add ShellRoute support to go_router_builder

[GP4cK]

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.

[flutter-dashboard[bot]]

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.

[GP4cK]

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.

[GP4cK]

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!

[kuyacai]

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');
  }

[GP4cK]

@kuyacai thanks a lot for this. I've refactored the generator so that it generates only a single $appRoutes in 7f4c6e0

[kuyacai]

ok, thx!

[azimuthdeveloper]

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?

[GP4cK]

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:

1. 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)
2. 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.

[azimuthdeveloper]

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

[chunhtai]

Hi @GP4cK can you help me understand why it needs to be const? I couldn't find the place the enforce it

[chunhtai]

I just learned from @johnpryan that annotation has to be const. Let me think a bit on what else we can do here...

[johnpryan]

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 Keys in Flutter aren't const, I'm not sure how we're going to support navigatorKey or parentNavigatorKey.

[kaankoken]

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:

1. 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)
2. 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.

[chunhtai]

@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

[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?

[kaankoken]

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

[GP4cK]

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;

[chunhtai]

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.