[go_router] fix PopScope.onPopInvokedWithResult getting called twice when popping a route if GoRouteData.onExit is not null

packages/go_router/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 15.1.2
+
+- Fixes `PopScope.onPopInvokedWithResult` getting called twice when popping a route if `GoRouteData.onExit` is not null
+
 ## 15.1.1
 
 - Adds missing `caseSensitive` to `GoRouteData.$route`.

packages/go_router/lib/src/delegate.dart

@@ -66,11 +66,12 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
     // Fallback to onExit if maybePop did not handle the pop
     final GoRoute lastRoute = currentConfiguration.last.route;
     if (lastRoute.onExit != null && navigatorKey.currentContext != null) {
-      return !(await lastRoute.onExit!(
-        navigatorKey.currentContext!,
-        currentConfiguration.last
-            .buildState(_configuration, currentConfiguration),
-      ));
+      return !(await lastRoute.onExit!.call(
+            navigatorKey.currentContext!,
+            currentConfiguration.last
+                .buildState(_configuration, currentConfiguration),
+          ) ??
+          true); // @TODO(techouse) not sure if returning true by default is correct
     }
 
     return false;
@@ -146,11 +147,14 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
     // a microtask in case the onExit callback want to launch dialog or other
     // navigator operations.
     scheduleMicrotask(() async {
-      final bool onExitResult = await routeBase.onExit!(
+      final bool? onExitResult = await routeBase.onExit!.call(
         navigatorKey.currentContext!,
         match.buildState(_configuration, currentConfiguration),
       );
-      if (onExitResult) {
+      if (onExitResult == null) {
+        route.didPop(result);
+      }
+      if (onExitResult ?? true) {
         _completeRouteMatch(result, match);
       }
     });
@@ -298,14 +302,17 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
       return SynchronousFuture<bool>(false);
     }
 
-    final FutureOr<bool> exitFuture = goRoute.onExit!(
+    final FutureOr<bool>? exitFuture = goRoute.onExit?.call(
       context,
       match.buildState(_configuration, currentConfiguration),
     );
     if (exitFuture is bool) {
       return handleOnExitResult(exitFuture);
     }
-    return exitFuture.then<bool>(handleOnExitResult);
+    return exitFuture?.then<bool>(handleOnExitResult) ??
+        SynchronousFuture<bool>(
+          true, // @TODO(techouse) not sure if returning true by default is correct
+        );
   }
 
   Future<void> _setCurrentConfiguration(RouteMatchList configuration) {

packages/go_router/lib/src/route.dart

@@ -67,7 +67,7 @@ typedef NavigatorBuilder = Widget Function(
 ///
 /// If the return value is true or the future resolve to true, the route will
 /// exit as usual. Otherwise, the operation will abort.
-typedef ExitCallback = FutureOr<bool> Function(
+typedef ExitCallback = FutureOr<bool>? Function(
     BuildContext context, GoRouterState state);
 
 /// The base class for [GoRoute] and [ShellRoute].
@@ -1453,6 +1453,7 @@ class _RestorableRouteMatchList extends RestorableProperty<RouteMatchList> {
 
   RouteMatchList get value => _value;
   RouteMatchList _value = RouteMatchList.empty;
+
   set value(RouteMatchList newValue) {
     if (newValue != _value) {
       _value = newValue;

packages/go_router/lib/src/route_data.dart

@@ -66,7 +66,7 @@ abstract class GoRouteData extends RouteData {
   /// Called when this route is removed from GoRouter's route history.
   ///
   /// Corresponds to [GoRoute.onExit].
-  FutureOr<bool> onExit(BuildContext context, GoRouterState state) => true;
+  ExitCallback? get onExit => null;
 
   /// A helper function used by generated code.
   ///
@@ -112,8 +112,8 @@ abstract class GoRouteData extends RouteData {
     FutureOr<String?> redirect(BuildContext context, GoRouterState state) =>
         factoryImpl(state).redirect(context, state);
 
-    FutureOr<bool> onExit(BuildContext context, GoRouterState state) =>
-        factoryImpl(state).onExit(context, state);
+    FutureOr<bool>? onExit(BuildContext context, GoRouterState state) =>
+        factoryImpl(state).onExit?.call(context, state);
 
     return GoRoute(
       path: path,

packages/go_router/test/on_exit_on_pop_invoked_with_result_test.dart

@@ -0,0 +1,137 @@
+import 'package:flutter/material.dart';
+import 'package:flutter_test/flutter_test.dart';
+import 'package:go_router/go_router.dart';
+
+void main() {
+  testWidgets(
+    'PopScope onPopInvokedWithResult should be called only once',
+    (WidgetTester tester) async {
+      int counter = 0;
+
+      final GoRouter goRouter = GoRouter(
+        initialLocation: '/',
+        routes: <GoRoute>[
+          GoRoute(path: '/', builder: (_, __) => const DummyStatefulWidget()),
+          GoRoute(
+            path: '/page-1',
+            builder: (_, __) => PopScope(
+              onPopInvokedWithResult: (bool didPop, __) {
+                if (didPop) {
+                  counter++;
+                  return;
+                }
+              },
+              child: const Text('Page 1'),
+            ),
+          ),
+        ],
+      );
+
+      addTearDown(goRouter.dispose);
+
+      await tester.pumpWidget(MaterialApp.router(
+        routeInformationProvider: goRouter.routeInformationProvider,
+        routeInformationParser: goRouter.routeInformationParser,
+        routerDelegate: goRouter.routerDelegate,
+      ));
+
+      goRouter.push('/page-1');
+
+      await tester.pumpAndSettle();
+
+      expect(find.text('Page 1'), findsOneWidget);
+      expect(
+        goRouter.routerDelegate.currentConfiguration.matches.length,
+        equals(2),
+      );
+      expect(goRouter.routerDelegate.canPop(), true);
+
+      goRouter.routerDelegate.pop();
+
+      await tester.pumpAndSettle();
+
+      expect(counter, equals(1));
+    },
+  );
+
+  testWidgets(
+    r'PopScope onPopInvokedWithResult should be called only once with GoRouteData.$route',
+    (WidgetTester tester) async {
+      int counter = 0;
+
+      final GoRouter goRouter = GoRouter(
+        initialLocation: '/',
+        routes: <GoRoute>[
+          GoRoute(path: '/', builder: (_, __) => const DummyStatefulWidget()),
+          GoRouteData.$route(
+            path: '/page-1',
+            factory: (GoRouterState state) => _Page1(
+              onPop: () {
+                counter++;
+              },
+            ),
+          ),
+        ],
+      );
+
+      addTearDown(goRouter.dispose);
+
+      await tester.pumpWidget(MaterialApp.router(
+        routeInformationProvider: goRouter.routeInformationProvider,
+        routeInformationParser: goRouter.routeInformationParser,
+        routerDelegate: goRouter.routerDelegate,
+      ));
+
+      goRouter.push('/page-1');
+
+      await tester.pumpAndSettle();
+
+      expect(find.text('Page 1'), findsOneWidget);
+      expect(
+        goRouter.routerDelegate.currentConfiguration.matches.length,
+        equals(2),
+      );
+      expect(goRouter.routerDelegate.canPop(), true);
+
+      goRouter.routerDelegate.pop();
+
+      await tester.pumpAndSettle();
+
+      expect(counter, equals(1));
+    },
+  );
+}
+
+class _Page1 extends GoRouteData {
+  const _Page1({
+    required this.onPop,
+  });
+
+  final VoidCallback onPop;
+
+  @override
+  Page<void> buildPage(BuildContext context, GoRouterState state) =>
+      MaterialPage<void>(
+        child: PopScope(
+          onPopInvokedWithResult: (bool didPop, __) {
+            if (didPop) {
+              onPop();
+              return;
+            }
+          },
+          child: const Text('Page 1'),
+        ),
+      );
+}
+
+class DummyStatefulWidget extends StatefulWidget {
+  const DummyStatefulWidget({super.key});
+
+  @override
+  State<DummyStatefulWidget> createState() => _DummyStatefulWidgetState();
+}
+
+class _DummyStatefulWidgetState extends State<DummyStatefulWidget> {
+  @override
+  Widget build(BuildContext context) => Container();
+}

packages/go_router_builder/example/lib/on_exit_example.dart

@@ -40,25 +40,25 @@ class SubRoute extends GoRouteData {
   const SubRoute();
 
   @override
-  Future<bool> onExit(BuildContext context, GoRouterState state) async {
-    final bool? confirmed = await showDialog<bool>(
-      context: context,
-      builder: (_) => AlertDialog(
-        content: const Text('Are you sure to leave this page?'),
-        actions: <Widget>[
-          TextButton(
-            onPressed: () => Navigator.of(context).pop(false),
-            child: const Text('Cancel'),
+  ExitCallback get onExit => (BuildContext context, GoRouterState state) async {
+        final bool? confirmed = await showDialog<bool>(
+          context: context,
+          builder: (_) => AlertDialog(
+            content: const Text('Are you sure to leave this page?'),
+            actions: <Widget>[
+              TextButton(
+                onPressed: () => Navigator.of(context).pop(false),
+                child: const Text('Cancel'),
+              ),
+              ElevatedButton(
+                onPressed: () => Navigator.of(context).pop(true),
+                child: const Text('Confirm'),
+              ),
+            ],
           ),
-          ElevatedButton(
-            onPressed: () => Navigator.of(context).pop(true),
-            child: const Text('Confirm'),
-          ),
-        ],
-      ),
-    );
-    return confirmed ?? false;
-  }
+        );
+        return confirmed ?? false;
+      };
 
   @override
   Widget build(BuildContext context, GoRouterState state) => const SubScreen();