Skip to content

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

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

Closed

Conversation

techouse
Copy link

@techouse techouse commented Mar 23, 2025

This pull request includes changes to the GoRouter package, focusing on improving the handling of route pop events and adding corresponding tests. The most important changes include modifying the GoRouterDelegate class to invoke the didPop method and adding comprehensive test cases to ensure the correct behavior of the PopScope widget.

The problem

PopScope.onPopInvokedWithResult is getting called twice when popping a route since v14.0.0 which introduced #6495.

The 1st time it's called from

https://github.com/flutter/flutter/blob/bb81a93d752690bc2889a713ec448359c9867f40/packages/flutter/lib/src/widgets/navigator.dart#L5586

The 2nd time it's called from

https://github.com/flutter/flutter/blob/bb81a93d752690bc2889a713ec448359c9867f40/packages/flutter/lib/src/widgets/navigator.dart#L3305

Before v14.0.0, the 2nd call doesn't happen.

Version PopScope.onPopInvokedWithResult called
12.1.3 1x
13.2.4 1x
14.0.0 2x
14.8.1 2x

I pinned it down to GoRouteData.onExit

FutureOr<bool> onExit(BuildContext context, GoRouterState state) => true;
returning true by default (it can never be null.

Since I use go_router_builder all my routes extend from GoRouteData where GoRouteData.onExit always a callable and never null

FutureOr<bool> onExit(BuildContext context, GoRouterState state) =>
so
if (routeBase is! GoRoute || routeBase.onExit == null) {
is never executed, as it would be had I created my GoRoutes manually without onExit defined.

Fixes:

Improvements to route pop handling:

Addition of test cases:

Pre-Review Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene page, which explains my responsibilities.
  • 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.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I linked to at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.
  • I updated/added any relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or I have commented below to indicate which test exemption this PR falls under1.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2 3

@techouse techouse changed the title [go_router] fix PopScope.onPopInvokedWithResult getting called twice when popping a route when GoRouteData.onExit is not null [go_router] fix PopScope.onPopInvokedWithResult getting called twice when popping a route if GoRouteData.onExit is not null Mar 23, 2025
@techouse
Copy link
Author

techouse commented Mar 24, 2025

@chunhtai Even though all the test pass, including the ones I specifically added to test this case, I am not 100% certain that this is the correct approach, however, I don't see another way, as GoRouteData.onExit is always a callable and never null, even though I don't explicitly declare it.

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change lgtm, but will need to fix version

@chunhtai chunhtai requested a review from hannah-hyj March 25, 2025 21:21
@chunhtai
Copy link
Contributor

looks like the ci is not happy

@techouse
Copy link
Author

techouse commented Mar 26, 2025

Change lgtm, but will need to fix version

The currently published version is 14.8.1, so I guess this one should be 14.8.2

@techouse
Copy link
Author

looks like the ci is not happy

Yeah, I'm not sure why. 🫠

@techouse techouse requested a review from chunhtai March 30, 2025 19:08
@chunhtai
Copy link
Contributor

chunhtai commented Apr 1, 2025

looks like some tests are failing https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8718793893833895121/+/u/Run_package_tests/Dart_unit_tests/stdout

@techouse
Copy link
Author

techouse commented Apr 1, 2025

looks like some tests are failing https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8718793893833895121/+/u/Run_package_tests/Dart_unit_tests/stdout

Oh, I see, the 1 test in the example directory is failing.

This basically points to a deeper problem. The onExit method is awaited and by the time it resolves the widget / screen is unmounted. Looks like I'll have to find a way to make GoRouteData.onExit truly null.

The only way I see this working is by changing ExitCallback from

typedef ExitCallback = FutureOr<bool> Function(
to

typedef ExitCallback = FutureOr<bool>? Function(BuildContext context, GoRouterState state);

but that opens up another can of worms where onExit can return null.

This will probably need a go_router_builder patch as well, since I changed

FutureOr<bool> onExit(BuildContext context, GoRouterState state) => true;

to

ExitCallback? get onExit => null;

and any override would have to be declared as

@override
ExitCallback? get onExit => (BuildContext context, GoRouterState state) {
  // stuff
  return true;
};

This quickly ballooned into a big task. 🫠

@chunhtai do you have any other suggestions perhaps?

@techouse techouse marked this pull request as draft April 1, 2025 18:51
@techouse techouse marked this pull request as ready for review April 8, 2025 18:27
@techouse
Copy link
Author

techouse commented May 7, 2025

Hi @chunhtai and @hannah-hyj

Would you mind taking another look at this PR?

The changes ended up being moderately substantial in the process of fixing the double onPopInvokedWithResult issue (especially around making onExit optional for GoRouteData). All the go_router tests are passing now, and the earlier failing example test highlighted a deeper onExit scenario that I’ll address in a separate PR for go_router_builder (to update its usage of the updated go_router version).

I’m committed to getting this fix landed and will follow up with the builder changes promptly.

Thank you both for your time and guidance! 

@chunhtai chunhtai self-requested a review May 8, 2025 18:56
@techouse techouse requested a review from chunhtai May 8, 2025 20:06
@techouse
Copy link
Author

techouse commented May 8, 2025

All tests passed, yay! 🎉

@chunhtai thank you for all the tips! 🙏

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I know what is going on, the issue is that when onPopPage returns false, it still attempt to call the onPopInvokedWithResult callback. This is something that needs to be fixed in framework

@chunhtai
Copy link
Contributor

chunhtai commented May 8, 2025

I will put up an framework pr to fix this closing this one

@chunhtai chunhtai closed this May 8, 2025
@chunhtai
Copy link
Contributor

chunhtai commented May 8, 2025

pr flutter/flutter#168567

@techouse techouse deleted the fix/double-call-onPopInvokedWithResult branch May 8, 2025 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants