-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[go_router] Support for top level onEnter
callback.
#8339
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
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
I agree that we should make one browser history entry for a logical “route change,” set to the final URL. Intermediate Navigation pipeline (confirmed):
// sugar for "redirect with replace semantics + loop detection"
Block(then: () => router.redirectTo('/final'));
This prevents authors from writing Restore/back: we re‑run We’ll reflect this in the migration guide: prefer |
and note that with my prev proposal, I am not trying to invent a duplicate of go(). |
sounds good to me. I will take a full review of this pr today |
_debugCheckParentNavigatorKeys( | ||
route.routes, | ||
<GlobalKey<NavigatorState>>[...allowedKeys..add(route.navigatorKey)], | ||
<GlobalKey<NavigatorState>>[...allowedKeys, route.navigatorKey], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch
packages/go_router/CHANGELOG.md
Outdated
## 16.3.0 | ||
|
||
- Adds new top level `onEnter` callback with access to current and next route states. | ||
- Deprecates top level `redirect` in favor of `onEnter`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't usually mark deprecation in package. instead, we just break the api and bump major version.
for this case though I think we should hold off deprecation until we have a better way to migrate the old behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the deprecation wording from the CHANGELOG and switched to calling the old behavior “legacy” in docs. I also removed the @Deprecated
annotations from the public redirect
API to avoid surfacing warnings before we have a clear migration path.
## 0.1.0 | ||
|
||
- squatting on the package name (I'm not too proud to admit it) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep the trailing new line
), | ||
), | ||
); | ||
} catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a demo purpose for this catch section? if no we should remove this part of code to avoid confusing reader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the extra catchError
and turned it into true fire‑and‑forget so it doesn’t block the guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also removed the try catch block.
if (_lastMatchList != null) { | ||
return SynchronousFuture<RouteMatchList>(_lastMatchList!); | ||
} | ||
// Fall back to parsing the current URI so Router still paints something sensible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems dangerous though, in this case won't we just access the page that is guarded by things like auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should just invoke onError call loop in this case, but we should make sure we document the behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Falling back to parsing the blocked target could display a guarded page. I changed the behavior to surface an error and route through onException
when there’s no prior route to restore, so we never add a blocked destination. I also added a doc comment explaining this.
infoState: infoState, | ||
onCanEnter: () { | ||
// Compose legacy top-level redirect here (one shared cycle/history) | ||
final Uri uri = RouteConfiguration.normalizeUri(effectiveRoute.uri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and other place, shouldn't we normalize at the place where we normailize the state? that way we don't need to tie up the lose end in different place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I now normalize the URI once when synthesizing RouteInformationState
so we don’t repeat normalization in downstream steps.
matchList = await onCanNotEnter(); | ||
|
||
// Hard stop (no then callback): reset history so user retries don't trigger limit | ||
final bool hardStop = result is Block && callback == null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a callback that does nothing can also be a hard stop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I documented the intent more clearly: Block()
(no then
) is the only guaranteed hard stop. We can’t reliably detect a “no‑op” closure, so Block(then: () {})
is treated as chaining by design. The Block docs now say this explicitly.
|
||
if (callback != null) { | ||
unawaited( | ||
Future<void>.microtask(callback).catchError(( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious why we do a microtask here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I might need to schedule then
via microtask
to avoid re‑entrancy into the parse pipeline (e.g., if the callback navigates or opens a dialog during build), and we report errors without undoing the committed navigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is close to tail, I don't think it worth to introduce another async gap just make sure it run before return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the callback is not run as part of onEnter, I think we should be fine
nice! @chunhtai Would you be interested in introducing a |
let's do it in another pr. I am still not sure what may be the best way to solve this. If we do a big refactor and move everything to routerdelegate, we may not have this issue at all. but in the same time it may require more work and be a breaking change |
|
||
/// Top level page redirect. | ||
/// Legacy top level page redirect. | ||
/// This is handled via [applyTopLegacyRedirect] and runs at most once per navigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a new line in between first and second paragraph
/// Processes redirects by returning a new [RouteMatchList] representing the new | ||
/// location. | ||
/// Processes route-level redirects by returning a new [RouteMatchList] representing the new | ||
/// location. This method now handles ONLY route-level redirects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our styleguide suggest first paragraph must be a brief one sentence describe the functionality. other supplementary doc needs to be another paragraph with a new line in between
final class Block extends OnEnterResult { | ||
/// Creates a [Block] result. | ||
/// | ||
/// The [then] callback is executed after the navigation is blocked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comment should be on the class's doc, the constructor doc usually used for things like what can be null and what not. basically focuses on the input restriction
|
||
/// Executed after the decision is committed. | ||
/// Errors are reported and do not revert navigation. | ||
final FutureOr<void> Function()? then; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to type def this
/// Note: We don't introspect callback bodies. Even an empty closure | ||
/// (`Block(then: () {})`) counts as chaining. Omit `then` entirely when you | ||
/// want the hard stop behavior. | ||
const Block({super.then}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider split this up into
Block.then(Then this.then)// force none null then callback
and
Block.stop()
|
||
if (callback != null) { | ||
unawaited( | ||
Future<void>.microtask(callback).catchError(( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is close to tail, I don't think it worth to introduce another async gap just make sure it run before return.
|
||
if (callback != null) { | ||
unawaited( | ||
Future<void>.microtask(callback).catchError(( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the callback is not run as part of onEnter, I think we should be fine
packages/go_router/pubspec.yaml
Outdated
- deep-linking | ||
- go-router | ||
- navigation | ||
- navigation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a new line at the end
/// behavior. | ||
final class Block extends OnEnterResult { | ||
/// Creates a [Block] that stops navigation without running a follow-up | ||
/// callback. Resets the redirection history so the next attempt is evaluated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new line in between the first paragraph and next
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't parse this doc.
Maybe just say
Returning a object of this constructor during onEnter callback halts the navigation completely.
const Block.stop() : super(); | ||
|
||
/// Creates a [Block] that runs [then] after the navigation is blocked. | ||
/// Keeps the redirection history to detect loops during chained redirects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new line in between the first paragraph and next
|
||
/// Blocks the navigation from proceeding. | ||
/// | ||
/// Use [Block.stop] for a "hard stop" that resets the redirection history, or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here maybe something along the line
Returning a object of this constructor during onEnter callback halts the navigation completely.
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
// ignore_for_file: use_build_context_synchronously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not do ignore for file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whenever this lint shows up, try to check for context.isMounted
before continue the operation
// Treat `Block.stop()` as the explicit hard stop. | ||
// We intentionally don't try to detect "no-op" callbacks; any | ||
// Block with `then` keeps history so chained guards can detect loops. | ||
final bool hardStop = result is Block && callback == null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider add a bool getter isStop on Block class so that we don't infer from callback== null
Adds context.mounted checks in GoRouteInformationParser and _OnEnterHandler to prevent actions on disposed contexts, improving safety during navigation and error handling. Enhances documentation for OnEnterResult and Block classes to clarify navigation blocking behavior. Removes unnecessary ignore_for_file: use_build_context_synchronously directives.
Description:
This PR introduces top level
onEnter
callback inRouteConfiguration
to 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:
onEnter
callback (typedef OnEnter
) to intercept route navigation before processing.onEnter
inGoRouteInformationParser
.RouteConfiguration
to include theonEnter
callback.top_level_on_enter.dart
, to demonstrate the feature.onEnter
callback 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.yaml
with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.md
to add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes].///
).