[go_router] Support for top level onEnter callback.
#8339
Conversation
Added onEnter callback to enable route interception and demonstrate usage in example.
|
Hi @chunhtai, This PR introduces a top-level onEnter callback, addressing critical gap in go_router’s handling of deep links and redirect customizations. It allows developers to intercept routes, block navigation, or implement custom logic. Given its potential impact, your feedback would be greatly appreciated. Thank you for your time! |
…nd enhanced OnEnter test.
|
Hi @chunhtai, Following up on this PR. I've updated the implementation to:
typedef OnEnter = bool Function(
BuildContext context,
GoRouterState currentState,
GoRouterState nextState
);
Let me know if you'd like any adjustments to this approach. |
Provides access to GoRouter within OnEnter callback to support navigation during early routing stages when InheritedGoRouter is not yet available in the widget tree.
|
Hi @chunhtai, I've expanded the The implementation:
|
|
The callback signature makes sense to me. The fallback router makes less sense to me. Since it's not the right instance, why would one use it ? Could you add its usage in the example ? |
|
Hi @cedvdb, The callback signature is indeed straightforward. The fallback router isn’t ideal in that it isn’t coming directly from the widget tree (via GoRouter.of(context)), but it serves as a backup when the Inherited widget isn’t available yet—typically on app startup. In those cases, if you try to look up the router from context, you’d get null because the tree isn’t fully built. Instead, the fallback (usually provided as this when constructing the parser) lets your onEnter callback still have a router reference so you can, for example, call router.go(...) to redirect or modify navigation. Here’s an example usage: GoRouter(
onEnter: (context, currentState, nextState, router) {
// Even if GoRouter.of(context) is null (because the tree isn’t built yet),
// the fallback router will be non-null.
if (nextState.uri.path == '/referral') {
// Use the router instance (either from context or fallback) to navigate
router.go('/handle-referral');
return false; // Prevent default navigation.
}
return true;
},
// other config...
)So while it isn’t “the right instance” from context when the tree isn’t ready, it’s our best–available router reference. If someone’s using the router during startup (or in any context where the inherited widget isn’t available), the fallback router provides a safe way to still programmatically navigate. The idea is that the fallback isn’t a “dummy” or incomplete instance—it’s the same router instance (typically provided as If you think this fallback might lead to unexpected behavior, one option is to document it clearly and advise that it’s only intended as a temporary solution until the tree is built. Otherwise, it’s our practical solution for early navigation calls. |
My bad, I misread the code and thought the fallback router was a parameter of the Router constructor, that we had to define ourselves, but that was the RouterInformationParser constructor I was reading. I will try this out on a decently sized project |
| ); | ||
|
|
||
| /// The signature of the onEnter callback. | ||
| typedef OnEnter = bool Function( |
There was a problem hiding this comment.
the conclusion is that we should return a resolution class where the redirect login will a callback in the returned object.
This will help us to only run the redirection after we get the resolution. I think this will reduce a lot of obscure corner case where the another round of redirection starts before the previous round has finished
There was a problem hiding this comment.
In general, stateful behaviors in navigation leads to unpredictable behavior on the web, that is: on the web an user can simply refresh a page and the state will be lost. Stateless redirection and re-redirection is achievable through query parameters (and is standard on the web)
I guess you could make the case that not every one has the web as a target and a stateful redirect is convenient. Still, being restrictive here by requiring statelessness should be considered imo
Maybe an enum may be more understandable than a boolean, else you gotta remember if true means "handled" or "you should handle it" and refer to the doc every time. "stop" and "next" are clearer.
There was a problem hiding this comment.
@cedvdb yes while a class-based result might seem more expressive initially, it introduces statefulness that can be problematic on the web. We should lean toward a design that is inherently stateless, leveraging the URL (and query parameters) to persist any transient state needed during redirection.
There was a problem hiding this comment.
yeah, if it has to return a stateless object, then using enum will be more preferable then boolean. I vaguely remembered I made this decision because there may be resolution other than a yes and no if we were to expand this API.
As return the class, or more specifically doing the redirection in the callback is that i want to avoid corner cases like this
Resolution onEnter(...) {
router.go('other route');
await somethingasync();
return false;
}
when this onEnter is called, the router.go will trigger the onEnter again before the entire redirection pipeline is finished, it is basically a non-tail recursion.
If we do this
Resolution onEnter(...) {
return Resolution.stop(callBack: () {
router.go('other route');
await somethingasync();
});
}
the route parser can finishes the redirecting pipeline before calling the callback, which will end up being a tail recursion.
My assumption is the non-tail recursion can create obscure bug.
There was a problem hiding this comment.
I agree that returning a plain boolean limits us to a yes/no decision, and as you've noted, if we let the side effect (like calling router.go('other route')) happen directly in the callback, it can trigger a reentrant call before the entire redirection pipeline is finished. That non-tail recursion is a recipe for obscure bugs.
By having the callback return a Resolution—an enum that can represent more than just “yes” or “no” - we can include a callback (or similar mechanism) that defers the actual navigation change until after the redirect pipeline is fully processed. In other words, by returning something like:
Resolution.stop(callback: () async {
await somethingAsync();
router.go('other route');
});we ensure that the router.go call isn’t executed until after the parser has finished its work. This effectively makes the redirection pipeline tail-recursive, avoiding the pitfall of re-triggering onEnter mid-flight.
Also I think using an enum here is more future-proof, too .... it gives us room to add more nuanced resolutions if needed. This stateless design fits well with web navigation paradigms where state isn’t preserved across refreshes, and it avoids the pitfalls of having stateful redirection logic.
|
Imo onEnter should be made asynchronous instead. One may query a local cache inside for example. I sent a PR on your fork that doesn't break the synchronous future usage |
|
@cedvdb thanks for the review, I've reviewed the changes in my last commit, and I believe I've resolved the issues effectively:
@chunhtai Regarding the discussion about using an enum (or even a class-based resolution) for a more expressive API, Look at the changes guys and let me know ur thoughts. |
- Change onEnter callback signature to FutureOr<bool> for async operations. - Update _processOnEnter for asynchronous navigation decisions. - Use last successful match as fallback on blocked navigation, preventing recursion. - Remove deprecated fallbackRouter parameter. - Adjust redirect limit and loop detection for stable fallback endpoints. - Update tests for sync, async, and loop scenarios. - Improve documentation of design decisions.
|
hi @chunhtai, whats left for this PR to be merged ? |
|
I will take a look today, also you can use re-request review when you want me to take another look. Btw, there is some analyzer error |
- Updated copyright notice to include "All rights reserved" - Fixed formatting to match other files in the repository - Resolved merge conflicts from upstream changes
|
I left this two comments regarding the unresolved two topics. #8339 (comment) |
Oh, yes I did not get any notifications about them, I have replied to them now. |
…rect * New `onEnter` returns `OnEnterResult` (`Allow`/`Block` with optional `then`) * Legacy top-level `redirect` composed into `onEnter`; parser path simplified * Normalize URIs; skip `onEnter` on restore; clear vs keep history (`Block()` vs `Block(then)`) * Refresh example; update/add tests
|
Hi @chunhtai, Thanks for the review, here is a summary of what changed in the last commit:
|
| /// The result of an [onEnter] callback. | ||
| /// | ||
| /// This sealed class represents the possible outcomes of navigation interception. | ||
| sealed class OnEnterResult { |
There was a problem hiding this comment.
Thinking a bit more it feels adding OnEnterResult.allow and OnEnterResult.block may be more confusing than it worth, I suggest we remove them
| final Object infoState = routeInformation.state!; | ||
|
|
||
| if (state is! RouteInformationState) { | ||
| // If we're restoring (pop/back or restoring a branch stack), skip onEnter |
There was a problem hiding this comment.
This seems weird if we are going to replace redirect with onEnter. Should we still attempt to run onenter?
There was a problem hiding this comment.
skipping onEnter on restore/back is intentional. We still run legacy + route-level redirects to keep URLs correct, but we don’t re-guard restores. The guard for “going back” is GoRoute.onExit, not onEnter.
There was a problem hiding this comment.
Also I am afraid we might face issues with iOS back gestures like before with onExit on sub routes.
Yep — I kept RoutingConfig.redirect (deprecated) for back-compat, and I run it after the new top-level onEnter and before route-level redirects. The order right now is: top-level onEnter (new guard) Tests in on_enter_test.dart assert this and that onEnter short-circuits redirect when it blocks. reading the comments again I understand now u want to compose the legacy top-level redirect into onEnter and let the parser handle a single guard path. EDIT:
Tests assert Before I change anything, can you confirm these choices?
Why I still have a valid reason to skip restoration with onEnter.
|
tbh I’d like to land this first, then follow up to add route-level onEnter (which would run before route-level redirect). My vote: we ship this PR as-is and follow up with route-level onEnter. it won’t break existing apps, and it keeps this change focused (parser + top-level guard + tests). |
Simplified the OnEnterResult class documentation and removed factory constructors from the sealed class. Introduced type aliases for navigation callbacks and route information state in parser.dart, and updated method signatures to use these aliases for improved readability and type safety. Also streamlined fallback logic when navigation is blocked.
Yes
We should still run onEnter when restoration, because the onEnter can be Auth or something that prevent viewing based on session. We can't skip them.
This is a good point. I almost feels like Block has to have at least a .go. doing nothing will be the same as a go(prev.uri).
Can you explain a bit more? |
Instead of forcing a redirect on
You're right, my concern was misplaced. I was incorrectly equating
I'm aligned with your proposal now. Here's my plan for the next commit:
|
… the onEnter and redirect guard handling.
|
Hi @chunhtai I did adapt legacy top-level redirect into
|
Doing nothing should do nothing, not even a refresh otherwise you break the (implicit) contract that it should just stop navigation.
IMO this is not necessary and may even be confusing. |
|
Note that I didn’t wrap Instead, I run legacy top redirect once right after Parser now sequences: |
|
@omar-hanafy I have 2 questions
|
I tried implementing top‑level
To maintain stability while If u see this differently, let me know ur thoughts.
I think:
|
ah it is because the go needs to wait for one frame to register. This is not good. I have thought about moving the parsing logic to the router delegate instead of in route parser. Originally i thought put it in routeinformationparser is a good idea, now not so much. Let's forget reimiplementing the top level redirect for now, what do you think that an onEnter that redirect multiple time in one route change creates multiple browser history entries. or do you think it should only generate one entry that contains the redirecting result? I personally prefer the latter, but would like to hear your thought |
Description:
This PR introduces top level
onEntercallback inRouteConfigurationto allow developers to intercept and conditionally block navigation based on incoming routes. It adds an example (top_level_on_enter.dart) demonstrating its usage, such as handling referral code from/referral.What This PR Changes:
onEntercallback (typedef OnEnter) to intercept route navigation before processing.onEnterinGoRouteInformationParser.RouteConfigurationto include theonEntercallback.top_level_on_enter.dart, to demonstrate the feature.onEntercallback functionality.Simple usage example:
Impact:
Enables developers to implement route-specific logic, such as support for handling action-based deep links without navigation (160602)
Pre-launch Checklist:
dart format.)[go_router].pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes].///).