-
Notifications
You must be signed in to change notification settings - Fork 96
[Proposal] [Enhancement] Add BuildContext to redirect #184
Comments
I agree with you. Unfortunately the lack of context during the redirect phase is a limitation of the underlying Router API. |
It would be really useful though. Can't we pass some kind of locator to the The locator could be something like: typedef GoRouterLocator = T Function<T>(); and then we could use it like this in the state of a late final GoRouter goRouter = GoRouter(locator: context.read); |
Actually this is possible but it would mean moving the code which is inside The role of Just for future reference (since people look more into issues than PR): I don't even see when it would be useful to have the |
It would be very useful to have access to a locator through |
Let's discuss this here instead of in the PR, for future reference. What about using a function parameter? Use This approach is not more flexible, it is:
|
Well, it was just simple example. But if you define the routes as a final variable outside of the StatefulWidget, you can't do that since you can't pass the BuildContext to the routes. |
If we needed another proof that global variable are bad: DON'T: loginGoRoute = GoRoute(
path: '/login',
pageBuilder: (context, state) => MaterialPage<void>(
key: state.pageKey,
child: const LoginPage(),
),
redirect: (state) {
final loginInfo = context.read<LoginInfo>();
final loggedIn = loginInfo.loggedIn;
// the user is logged in and headed to /login, no need to login again
if (loggedIn) return '/';
// no need to redirect at all
return null;
},
); DO GoRoute loginGoRouteBuilder(BuildContext context) => GoRoute(
path: '/login',
pageBuilder: (context, state) => MaterialPage<void>(
key: state.pageKey,
child: const LoginPage(),
),
redirect: (state) {
final loginInfo = context.read<LoginInfo>();
final loggedIn = loginInfo.loggedIn;
// the user is logged in and headed to /login, no need to login again
if (loggedIn) return '/';
// no need to redirect at all
return null;
},
); The second case better in many cases anyway, what if you want to pass routes to your Here is an exampleGoRoute homeGoRouteBuilder(BuildContext context, List<GoRoute> routes) => GoRoute(
path: '/',
pageBuilder: (context, state) => MaterialPage<void>(
key: state.pageKey,
child: HomePage(families: Families.data),
),
routes: routes,
); Still not convinced? |
@letsar I'm sorry to be so contradictory. I think your PR was great, which documentation, README update and example, and would have loved to accept it. However, since I am more concerned about the usefulness of any feature increasing the API surface, I need to be convinced that there is a real need. |
Global immutable variables are not bad, look at how riverpod is built. But this is not the subject. By transforming our variable held somewhere in a class or at the top-level, by a function needing a BuildContext, we just make things really hard to implement since we have to pass the context everywhere. And by the way, the solution provided doesn't prevent anyone to do it like you suggested if they prefer. It only add a simple way to get dependencies from any route no matter where they are defined. It's good to have contradictory point of vue, it's challenging and helps to see if something is really necessary. In a big app with different modules being independent and each module defining its own navigation, it would take a lot of work to refactor everything by passing a BuildContext everywhere and in my opinion it adds more complexity in the codebase. |
Agree to disagree, I like to pass my dependencies manually to make them transparent but I agree it's more verbose than passing dependencies by DI (which is basically what you do in Just to clarify 2 things that you seem to have misunderstood:
I agree, I meant that here they are bad because they prevent you from passing dependencies around (look at my second example with
It's not just the code added to the package in itself but the number of concept of a package. This is one more attribute, some more documentation to read. You might say that people can ignore it but having too much things are quite overwhelming, especially for newcomers. Now that that's said, maybe you could argue that |
I agree that it would be better in fact to have access to the But in order to do this, as you said, we need to have a dedicated |
This would be a breaking change because the signature of the This would not be a breaking change for the reasons you cited. The change from Where is the idea: // BEFORE
class GoRouterDelegate extends ... {
...
// No access to `context` here, so `redirect` can't have a `context`
@override
build(BuildContext context) {
return Navigator(...);
}
}
// AFTER
class GoRouterDelegate extends ... {
...
@override
build(BuildContext context) {
return _GoRouterDelegateWidget(...);
}
}
class _GoRouterDelegateWidget extends StatefulWidget {
...
const _GoRouterDelegateWidget({Key? key, ...}) : super(key: key);
@override
_GoRouterDelegateWidgetState createState() => _GoRouterDelegateWidgetState();
}
class _GoRouterDelegateWidgetState extends State<_GoRouterDelegateWidget> {
// Here we have a `context`
@override
build(BuildContext context) {
return Navigator(...);
}
}
|
I don't understand the before/after. For the moment |
Sorry the example was not complete as Anyway regarding your question:
This is right, right and wrong:
|
Oh Thanks ! I missed it ! |
However what So if we use |
Ok I'm starting to see where it can lead, but I'm not sure it would be easily doable. For example, the |
I agree that it would not be as simple as moving everything to |
I'm loving this discussion :) I have a quick question. When we call If I am, why not passing that context all the way down? Now, there is at least one place when the At that point, we could have access to a build context if the constructor of the GoRouter requires one, from there on, we can keep passing the context down up to the redirectors and whenever they get triggered they will have the deepest context available from whatever triggers them (like I said, a call that makes I haven't had time to sit and test @letsar 's PR properly, but the solution seems very limited to me. Making the whole thing a widget however makes so much more sense to me (we would wrap the material app with GoRouter and use a builder that passes down the router and delegate to it). However, I still fail to see how this change can help us get the deepest level context available when redirecting? Like I mentioned in my example, I might navigate to a page that requires certain permission by just using the browser and forcing the url to go there, the redirect should prevent me from that. Let's see this example: /user/42/profile (this goes to the profile page of user with id 42) At that point I did a navigation that in my builder I wrapped my page in a bloc provider that instantiated a new user profile bloc with the id 42. This is a bloc that exists in the widget tree deep down and as soon as I pop the profile page it gets disposed. Let's say now that some users have an album of pictures that they can set as public of private. /user/42/profile/album This is the route to the album. Is a page to explore the pictures. In my page I can simply hide the button for going to the album's page by checking the flag on the user's model, that's fine, and for mobile is perfect but on web, using the url strategy path, I can force that URL and open the album from any user regardless of the album's flag. I can try to just double check (the button visibility in the profile check + check again when the page is going to render and show an error if the flag doesn't allow viewing) but I believe this is not a good experience. The user should be automatically redirected to the previous page (/user/42/profile) and that's it. Forcing the url always results in the same thing. Alternatively, we could just redirect to an error page, but never let the user see the url they typed and an error screen that could cause confusion. Like, if I'm not logged in to my Facebook account but I force the url to a private post, I won't see the post and the url might be redirected to some other page. Anyways, regardless of how it's done, having access to at least the top level context (above material app at least) that we can have access to injected dependencies even more above the router would be enough for me to say it covers 80% of the most common use cases. |
Perhaps Widget build(BuildContext context) => MaterialApp(builder: (context) => _router); In that case, the redirect could have a context from the router itself. In theory, this would remove the need for a |
As far as I know this is not possible because |
@lulupointu I'm not sure what "report de url" means. |
Oh nvm if you use the What I meant is that |
Remi's idea in more detail: class GoRouter extends StatefulWidget {
const GoRouter({Key? key}) : super(key: key);
@override
_GoRouterState createState() => _GoRouterState();
}
class _GoRouterState extends State<GoRouter> {
final delegate = _GoRouterDelegate();
@override
Widget build(BuildContext context) => Router(routerDelegate: delegate, ...);
@override
void dispose() {
delegate.dispose();
super.dispose();
}
@override
didUpdateWidget(oldWidget) {
delegate.refresh();
}
...
}
void main() => runApp(MaterialApp(builder: (context, _) => GoRouter(...))); This would enable building the router itself conditionally: MaterialApp(
builder: (context, _) => GoRouter(
routes: [
GoRoute(path: '/login', ...),
if (isLoggedIn) GoRoute(path: '/', ...),
],
),
) This would also allow provide a Widget build(_) => MaterialApp(builder: (_) => _router);
final _router = GoRouter(
...,
redirect: (context, state) {
final loginInfo = LoginInfo.of(context); // rebuild when LoginInfo changes
...
}
);``` |
Yes that would be pretty great indeed! |
We could even define a class GoRouter extends StatefulWidget {
const GoRouter({Key? key}) : super(key: key);
static WidgetBuilder build({Key? key, ...}) => ((_, __) => GoRouter(...));
@override
_GoRouterState createState() => _GoRouterState();
} To be used like so: void main() => runApp(MaterialApp(builder: GoRouter.build(...))); |
For what it's worth, I really like the sound of making GoRouter a Widget :) |
Personally I love the idea! @lulupointu I would go for MaterialApp(builder: GoRouter.builder(...)) Maybe we could even have a
So it would be like GoRouter(
data: GoRouterData(...),
); final GoRouterController routerController = GoRouter.of(context); |
|
https://github.com/csells/go_router/blob/master/lib/src/go_router_delegate.dart#L350
When we use the redirect we might want to check for dependencies injected in the widget tree using the context.
For instance, I have pages that are only accessible under certain user permissions. In Flutter web a user can simply type the url path to the page they want to see (navigate) but if the permission is not present the page should redirect.
I have the permissions up in the tree as the state of a bloc (using flutter bloc) and it would be great if I could receive the context in the redirect and simply find the bloc's state.
I believe this is possible, either passing the current navigator's state's context to the redirect callback or to the GoRouterState.
The text was updated successfully, but these errors were encountered: