Closed nosmirck closed 2 years ago
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 GoRouter
constructor that would be passed to the GoRouterDelegate
and ultimately provided by the redirect
callback?
The locator could be something like:
typedef GoRouterLocator = T Function<T>();
and then we could use it like this in the state of a StatefulWidget
(if using provider):
late final GoRouter goRouter = GoRouter(locator: context.read);
Actually this is possible but it would mean moving the code which is inside GoRouter
into a (statefull) _GoRouterWidget
.
The role of GoRouter
would then only be to create this _GoRouterWidget
(and to pass down the attributes + the notifyListener
callback).
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 context
(or locator
) in the redirect
callback since you already have a context
in the State
holding GoRouter
.
It would be very useful to have access to a locator through GoRouterState
in different situations. You may want to have your routes in a final variable outside of any StatefulWidget
and since each GoRoute
has a redirect function, we can also use the locator in these places. You may also want to have the redirect function a top-level function. This is more flexible and powerful that putting the GoRouter
's instance in a StatefulWidget
.
Let's discuss this here instead of in the PR, for future reference.
What about using a function parameter? Use void _redirect(BuildContext context, GoRouterState state) {...})
?
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 GoRoute
which are defined elsewhere?
@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. Here I don't think that it adds that much complexity in the codebase of go_router and yet if offers a really simple way to resolve the solution.
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 locator
, I think you know that looking at the name you chose :sweat_smile: )
Just to clarify 2 things that you seem to have misunderstood:
Global immutable variables are not bad, look at how riverpod is built
I agree, I meant that here they are bad because they prevent you from passing dependencies around (look at my second example with homeGoRouteBuilder
I don't think that it adds that much complexity in the codebase of go_router
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 redirect
functions nearly always need the context
because of what they are: they look at the outside dependencies vs the GoRouterState
and make a decision based on that.
With which I agree, and therefore might consider adding a context
variable to redirect
(as I already said was possible). I would not do this for a package developed for my personal use but since it's a public package. Let's see what others think :)
I agree that it would be better in fact to have access to the BuildContext
from the redirect function, since it's more flexible.
But in order to do this, as you said, we need to have a dedicated StatefulWidget
to instantiate GoRouter
(let's call it GoRouterProvider
) and pass its BuildContext
to the GoRouterDelegate
. It would be a major breaking change since people would have to use this new GoRouterProvider
widget to create their GoRouter
and change all their redirect function to accept the new passed BuildContext
.
This would be a breaking change because the signature of the redirect
function would change.
This would not be a breaking change for the reasons you cited. The change from GoRouterDelegate
to GoRouterDelegate + _GoRouterDelegateWidget
would not be visible from the user perspective.
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 GoRouterDelegate
is not a widget and already extends RouterDelegate<Uri>
, thus it doesn't have a build method and can't have one. What did I miss?
Sorry the example was not complete as _GoRouterDelegateWidget
needed to be statefull. I updated that.
Anyway regarding your question:
For the moment
GoRouterDelegate
is not a widget and already extendsRouterDelegate<Uri>
, thus it doesn't have a build method and can't have one
This is right, right and wrong:
GoRouterDelegate
is not a widgetGoRouterDelegate
does extends RouterDelegate<Uri>
GoRouterDelegate
does have a build
methodwrong: GoRouterDelegate does have a build method
Oh Thanks ! I missed it !
It still don't see how the GoRouterDelegate
could be inserted in the widget tree like that, since GoRouter
has the responsibility to create the GoRouterDelegate
and GoRouter
is not a widget, can you elaborate?
RouterDelegate
is not a widget so is can never be part of the widget tree.
However what RouterDelegate
does is created the subtree of the WidgetApp
is in given to (see the Router
source code which calls RouterDelegate.build
, and WidgetApp
source code which create a Router
)
So if we use _GoRouterDelegateWidget
in GoRouterDelegate.build
, _GoRouterDelegateWidget
will be in the widget tree and therefore _GoRouterDelegateWidgetState
will have a valid context
as any other widget.
Ok I'm starting to see where it can lead, but I'm not sure it would be easily doable. For example, the RouterDelegate
overrides setInitialRoutePath
. GoRouterDelegate
calls a _go
method in there that checks first for redirections. Since we're not yet in the StatefulWidget
, we would have to use some kind of controller, passed to the _GoRouterDelegateWidget
, to do the job and pass its BuildContext
to redirect methods.
I agree that it would not be as simple as moving everything to _GoRouterDelegateWidget
. However this is definitely possible by separating the role of GoRouterDelegate
(which would only be to expose the current location, notifying its listener when needed), and _GoRouterDelegateWidget
the remaining (navigation, routing, matching, redirection ...). I don't think this would require any controller, if this is well design it should only need the passing down the current location and a method to change the url from GoRouterDelegate
to _GoRouterDelegateWidget
, and a good implementation of _GoRouterDelegateWidgetState.didUpdateWIdget
to react to location
changes
I'm loving this discussion :)
I have a quick question.
When we call context.go('/path')
(And any other method like goNamed) which will at some point converge to a call to that _go
method is a chained, nested sequence of calls that, at some point, call the redirect, am I correct?
If I am, why not passing that context all the way down?
Now, there is at least one place when the _go
method is called without coming from a context, and that's only the very first navigation to the initial route in the delegate's constructor.
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 _go
call)
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 GoRouter
itself could be a widget? In that case, you could use it like so:
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 refreshListener
too. This was @rrousselGit's idea and I like it.
As far as I know this is not possible because MaterialApp
will report de url, therefore GoRouter
will not be able too. At least this was the case a few Months ago but maybe something changed.
@lulupointu I'm not sure what "report de url" means.
Oh nvm if you use the builder
it works !
What I meant is that MaterialApp(home: GoRouter(...))
would not work. But it seems that MaterialApp(builder: (_, __) => GoRouter(...))
does so that's great !
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();
}
...
}
GoRouter
initialization would change:
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 context
for use in redirect
:
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 build
builder on GoRouter
like so:
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
GoRouter
Change notifier. It is good to have a change notifier if we want to listen to route changes.(update: It could be actually the GoRouterDelegate so we can use GoRouterState)So it would be like
GoRouter(
data: GoRouterData(...),
);
final GoRouterController routerController = GoRouter.of(context);
GoRouter.builder
already exists that's why I used GoRouter.build
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.