[go_router] Fix redirect zone async error handling - #12836
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request replaces runZonedGuarded with Zone.current.fork().run in RouteConfiguration to prevent unrelated async errors during redirects from being wrapped as GoException. It also adds a corresponding test case and a changelog entry. The reviewer noted that this change introduces a regression because synchronous exceptions thrown by the redirect callback are no longer wrapped in a GoException. They suggested wrapping the execution in a try-catch block to preserve the original error-wrapping behavior.
| return Zone.current | ||
| .fork(zoneValues: <Object?, Object?>{currentRouterKey: router}) | ||
| .run<T>(callback); |
There was a problem hiding this comment.
By removing runZonedGuarded and directly returning the result of Zone.run, synchronous exceptions thrown by the redirect callback are no longer wrapped in a GoException (e.g., Exception during redirect: <error>). This is a regression in error handling behavior for synchronous redirect errors.
To preserve the wrapping of synchronous exceptions while still avoiding the issue with unrelated async errors, we can wrap the call in a try-catch block.
try {
return Zone.current
.fork(zoneValues: <Object?, Object?>{currentRouterKey: router})
.run<T>(callback);
} catch (error) {
throw error is GoException
? error
: GoException('Exception during redirect: $error');
}There was a problem hiding this comment.
_runInRouterZone is only called from the top-level and route-level redirect handlers, and both call sites already wrap synchronous
exceptions in GoException. The top-level path wraps with Exception during redirect, while the route-level path wraps with Exception during route redirect.
The change here keeps _runInRouterZone focused on providing the router zone value only, while preserving synchronous and returned-Future redirect error
handling at the existing call sites. The added regression test covers the unrelated unawaited async error case from the issue.
Fixes flutter/flutter#192066
Summary
Replaces the guarded redirect zone with a value-only zone when running redirect callbacks.
Redirect callbacks still have access to the current router through zone values, so APIs such as
GoRouter.of(context)andcontext.namedLocation(...)continue to work during redirects. However, unrelated unawaited async errors started during a redirect are no longer caught by go_router and wrapped as
GoException.Tests
fvm dart run ../../script/tool/bin/flutter_plugin_tools.dart format --packages go_routerfvm dart run ../../script/tool/bin/flutter_plugin_tools.dart analyze --packages go_routerfvm dart run ../../script/tool/bin/flutter_plugin_tools.dart dart-test --packages go_routerfvm dart run ../../script/tool/bin/flutter_plugin_tools.dart validate --packages go_routerfvm flutter test test/go_router_test.dartfvm flutter test test/redirect_chain_test.dartFor the checklist, check the ones that are true: