[go_router] adds async feature and build context to redirect #2435
Conversation
|
This pull request was opened against a branch other than main. Since Flutter pull requests should not normally be opened against branches other than main, I have changed the base to main. If this was intended, you may modify the base back to go_router_v5. See the Release Process for information about how other branches get updated. Reviewers: Use caution before merging pull requests to branches other than main, unless this is an intentional hotfix/cherrypick. |
chunhtai
force-pushed
the
issues/105808
branch
from
370fbef to
49e9515
Compare
|
The ci is failing due to flutter sdk version mismatched, may need some more time to figure out, otherwise the code is ready |
There was a problem hiding this comment.
Sychronous future can't be used in function that marked async, that is why I refactor the entire redirect pipeline.
There was a problem hiding this comment.
Please add a comment here explaining why SynchronousFuture is used instead of Future.value.
There was a problem hiding this comment.
I'd move that redirect comment up. Also, I'd capitalize all comments and add punctuation to all comments in this file for consistency.
| // log a user in, letting all the listeners know | |
| StreamAuthScope.of(context).signIn('test-user'); | |
| setState(() { | |
| loggingIn = true; | |
| }); | |
| // router will automatically redirect from /login to / using | |
| // refreshListenable | |
| // Log a user in and notify all listeners. | |
| // This will automatically redirect from /login to / using | |
| // refreshListenable. | |
| StreamAuthScope.of(context).signIn('test-user'); | |
| setState(() { | |
| loggingIn = true; | |
| }); |
There was a problem hiding this comment.
oops it was a outdated comment i forgot to delete, it no longer apply
There was a problem hiding this comment.
What do you think of making all these FutureOr? Benefits:
- Makes this breaking change a bit easier for folks migrating to v5
- Folks don't need to do anything special for the common synchronous scenario
Drawback: our code may be a bit more complicated.
This seems like a good trade-off given folks are unlikely to use SynchronousFuture when they should.
There was a problem hiding this comment.
It is also against flutter style guide https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-using-futureort
developer can also mark a synchronous method a async if they don't want to bother with SynchronousFuture, and it is not too bad, it will be introduce a one frame delay before page transition. I don't think this is worth it to breaking flutter style guide in this case.
chunhtai
force-pushed
the
issues/105808
branch
from
d4642a1 to
40e3de6
Compare
chunhtai
force-pushed
the
issues/105808
branch
from
40e3de6 to
7082a70
Compare
|
@loic-sharma PTAL :) |
| You can use redirection to prevent the user from visiting a specific page. In | ||
| go_router, redirection can be asynchronous. | ||
|
|
||
| ```dart | ||
| GoRouter( | ||
| ... | ||
| redirect: (context, state) async { | ||
| if (await LoginService.of(context).isLoggedIn) { | ||
| return state.location; | ||
| } | ||
| return '/login'; | ||
| }, | ||
| ); | ||
| ``` |
There was a problem hiding this comment.
Async redirects is a bit more of an advanced scenario. I'd personally prefer to put the simpler sync scenario here and keep async to an example.
packages/go_router/README.md
Outdated
|
|
||
| If the code depends on [BuildContext](https://api.flutter.dev/flutter/widgets/BuildContext-class.html) | ||
| through the [dependOnInheritedWidgetOfExactType](https://api.flutter.dev/flutter/widgets/BuildContext/dependOnInheritedWidgetOfExactType.html) | ||
| (which is how `of` method is usually implemented), the redirect will be called every time the [InheritedWidget](https://api.flutter.dev/flutter/widgets/InheritedWidget-class.html) |
There was a problem hiding this comment.
| (which is how `of` method is usually implemented), the redirect will be called every time the [InheritedWidget](https://api.flutter.dev/flutter/widgets/InheritedWidget-class.html) | |
| (which is how `of` methods are usually implemented), the redirect will be called every time the [InheritedWidget](https://api.flutter.dev/flutter/widgets/InheritedWidget-class.html) |
| (which is how `of` method is usually implemented), the redirect will be called every time the [InheritedWidget](https://api.flutter.dev/flutter/widgets/InheritedWidget-class.html) | ||
| updated. | ||
|
|
||
| ### Top-level redirect |
There was a problem hiding this comment.
I don't think a newcomer would understand top-level vs route-level redirects by this point. Should we update the code sample above to contain both a top-level and route-level redirect?
| Widget build(BuildContext context) => MaterialApp.router( | ||
| routeInformationProvider: _router.routeInformationProvider, | ||
| routeInformationParser: _router.routeInformationParser, | ||
| routerDelegate: _router.routerDelegate, |
There was a problem hiding this comment.
Should we update this to use @johnpryan's RouterConfig changes once that's merged?
| bool routerNeglect, | ||
| ) { | ||
| List<Page<dynamic>>? pages; | ||
| if (matches.isEmpty) { |
There was a problem hiding this comment.
Consider adding a comment as to when this scenario happens. My guess is this includes async redirect, which isn't obvious.
| await tester.pumpWidget(MaterialApp.router( | ||
| routeInformationProvider: router.routeInformationProvider, | ||
| routeInformationParser: router.routeInformationParser, | ||
| routerDelegate: router.routerDelegate, |
There was a problem hiding this comment.
This guy can also be updated to use RouterConfig when that's ready
There was a problem hiding this comment.
LGTM! Great work! 🚀
Please also get a review from @johnpryan as my go router knowledge has huge gaps 😅
packages/go_router/CHANGELOG.md
Outdated
| - Fixes a bug where intermediate route redirect methods are not called. | ||
| - **BREAKING CHANGE** | ||
| - Redesigns redirection API. | ||
| - Redesigns redirection API, adds asynchronous feature, and builds context to redirect. |
There was a problem hiding this comment.
| - Redesigns redirection API, adds asynchronous feature, and builds context to redirect. | |
| - Redesigns redirection API, adds asynchronous feature, and adds build context to redirect. |
3 participants
fixes flutter/flutter#105808
fixes flutter/flutter#99121
Pre-launch Checklist
dart format.)[shared_preferences]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.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.