-
Notifications
You must be signed in to change notification settings - Fork 3.6k
go_router: add onEnter route callback #10898
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
go_router_builder: generate onEnter callback go_router_builder: add tests for onEnter generation
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.
Code Review
The pull request introduces route-level onEnter navigation guards for GoRouteData classes in go_router and go_router_builder. This feature allows developers to implement specific navigation guards for individual routes, aligning with the existing onExit pattern. The changes include adding an EnterCallback typedef, an onEnter parameter to the GoRoute constructor, and an onEnter method to _GoRouteDataBase with a default implementation. The go_router_builder has been updated to generate the onEnter callback, and new test cases demonstrate its usage. It's important to note that while the API surface and code generation are implemented, the routing infrastructure to invoke GoRoute.onEnter during navigation is not yet part of this PR and will require further updates to parser.dart.
PR_DESCRIPTION.md
Outdated
| - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. | ||
| - [x] I read the [Tree Hygiene] page, which explains my responsibilities. | ||
| - [x] I read and followed the [relevant style guides] and ran [the auto-formatter]. | ||
| - [ ] I signed the [CLA]. |
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.
| // https://github.com/flutter/flutter/issues/171410 | ||
| import 'package:meta/meta.dart' as meta; | ||
|
|
||
| import '../go_router.dart'; |
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 import ../go_router.dart seems redundant here. Typically, go_router.dart re-exports its public APIs, and specific files like configuration.dart, match.dart, on_enter.dart, etc., are imported directly. Importing the top-level library might lead to unnecessary dependencies or potential circular imports if not carefully managed. Please consider removing this import if all necessary symbols are already imported from their specific files.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| *List which issues are fixed by this PR. You must list at least one issue.* | ||
|
|
||
| ## Description |
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 file is used for all pull request, we shouldn't change it. Can you revert this change?
| ); | ||
| } | ||
|
|
||
| // (Moved) Route-level onEnter handling is implemented as an instance method |
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.
what does the (moved) mean in the context of this doc?
| if (routeLevelBlocked != null) { | ||
| // A route-level onEnter blocked navigation; treat as blocked. | ||
| matchList = routeLevelBlocked; | ||
| _resetRedirectionHistory(); |
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.
why do we reset history here?
|
|
||
| // Iterate in the same visitation order and invoke onEnter sequentially. | ||
| for (final RouteMatchBase match in routeMatches) { | ||
| if (!context.mounted) { |
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 think if context is not mounted, we should just abort the entire navigation
|
|
||
| if (onEnterResult is Block) { | ||
| // When a route-level onEnter blocks, run onCanNotEnter to get final match list. | ||
| final OnEnterThenCallback? thenCb = onEnterResult.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 should avoid uncommon abbreviation
| final OnEnterThenCallback? thenCb = onEnterResult.then; | |
| final OnEnterThenCallback? thenCallback = onEnterResult.then; |
| /// - [current]: The current route state | ||
| /// - [next]: The route state being navigated to | ||
| /// - [router]: The GoRouter instance | ||
| typedef EnterCallback = |
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.
There is already an OnEnter typedef
…type and improve navigation state reset logic and unmounted context handling.
go_router_builder: generate onEnter callback
go_router_builder: add tests for onEnter generation
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
Description
This PR adds support for route-level
onEnternavigation guards inGoRouteDataclasses forgo_router_builder.What changed
go_router:
EnterCallbacktypedef for route-level onEnter callbacksonEnterparameter toGoRouteconstructoronEntermethod to_GoRouteDataBasewith defaultAllow()implementation_GoRouteParametersto includeonEntercallbackGoRouteData.$route()andRelativeGoRouteData.$route()helpers to passonEnterto generated routesgo_router_builder:
on_enter.dartdemonstrating usage ofonEnterinGoRouteDataclassesonEntermethod through existing$route()helper infrastructureWhy
Resolves issue #181471
Currently,
go_router16.3.0 introduced a globalonEntercallback at theGoRouterlevel. However, there's no way to define route-specific navigation guards inGoRouteDataclasses, unlikeredirectandonExitwhich work at the route level.This PR enables developers to override the
onEntermethod in theirGoRouteDataclasses to implement navigation guards for specific routes, providing consistency with the existingonExitpattern.Example Usage
Note on Implementation Status
GoRoute.onEnterduring navigation is not yet implemented. The generated code is correct, but the callback won't be invoked until the routing logic inparser.dartis updated to handle route-levelonEntercallbacks (similar to howonExitis handled indelegate.dart).This PR can serve as a foundation, but additional work is needed to complete the feature.
Fixes #181471
Tests
go_routerpassgo_router_builderpass (including newon_enter.darttest case)packages/go_router_builder/test_inputs/on_enter.dartand.expectfileVersion Updates
CHANGELOG.mdfor bothgo_routerandgo_router_builderPre-Review Checklist
[go_router_builder]and[go_router]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.