Skip to content
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

[go_router] Redirection with Riverpod example #112915

Open
creativecreatorormaybenot opened this issue Oct 5, 2022 · 18 comments
Open

[go_router] Redirection with Riverpod example #112915

creativecreatorormaybenot opened this issue Oct 5, 2022 · 18 comments
Labels
c: proposal A detailed proposal for a change to Flutter d: examples Sample code and demos p: go_router The go_router package P3 Issues that are less important to the Flutter project package flutter/packages repository. See also p: labels. team-go_router Owned by Go Router team triaged-go_router Triaged by Go Router team

Comments

@creativecreatorormaybenot
Copy link
Contributor

creativecreatorormaybenot commented Oct 5, 2022

This is a request to add an example to the go_router package that shows how to set up redirection with Riverpod.

Use case

go_router is currently not very well documented and so it took me (as an individual who is familiar with how the different parts of the framework work) quite a while to figure out a solid way to use go_router with flutter_riverpod properly to handle a sign-in flow.

Proposal

Because of that, I feel like the existing example catalog should be updated with another sample to complement "Redirection" and "Asynchronous Redirection".
This is mostly because it works fairly differently under the hood.


Is this something you would be willing to integrate into the package? I would also be willing to contribute the example.
I think this would be very beneficial to also ensure that updates to the package do not break this setup.

Edit: sample here dartpad.dev & explanation here issuecomment-1269053217.

Setup

This method makes use of the fact that reconstructing GoRouter will call redirect again.
That means that by providing a redirect function that depends on providers, we can both handle calls to redirect caused by internal calls (Navigator.pop, context.go, etc.) as well as the providers updating.

It looks something like this:

// This is super important - otherwise, we would throw away the whole widget tree when the provider is updated.
final navigatorKey = GlobalKey<NavigatorState>();
// We need to have access to the previous location of the router. Otherwise, we would start from '/' on rebuild.
GoRouter? _previousRouter;

final routerProvider = Provider((ref) {
  final bool loggedIn = ref.watch(loggedInProvider);

  return GoRouter(
    initialLocation: _previousRouter?.location,
    navigatorKey: navigatorKey,
    routes: [...],
    redirect: (context, state) {
      // if the user is not logged in, they need to login
      final bool loggingIn = state.subloc == '/login';
      if (!loggedIn) {
        return loggingIn ? null : '/login';
      }

      // if the user is logged in but still on the login page, send them to
      // the home page
      if (loggingIn) {
        return '/';
      }

      // no need to redirect at all
      return null;
    },
  );
});

This setup is so much cleaner and easier to grasp than the existing Redirection example using provider.

Also note that I was not able to set this up using refreshListenable, which is not documented, confusing, and also does not work correctly from what I can tell (it can cause invalid states). Furthermore, the refreshListenable setup seems way more hacky than the riverpod setup above to begin with.

In conclusion, I think that this method is pretty powerful and I would like to see it included in the package publicly somehow in order to ensure that others can use it and it can be maintained (although it should always work if the package is implemented correctly and checks for equality properly without throwing away the widget tree).

@darshankawar darshankawar added in triage Presently being triaged by the triage team p: first party package flutter/packages repository. See also p: labels. c: proposal A detailed proposal for a change to Flutter p: go_router The go_router package d: examples Sample code and demos and removed in triage Presently being triaged by the triage team labels Oct 5, 2022
@TahaTesser
Copy link
Member

TahaTesser commented Oct 5, 2022

@creativecreatorormaybenot
Do you plan to add this example yourself? If not, I can take this.

(Since Riverpod is supposed to be a better version of Provider, I think adding Riverpod examples for both "Redirection" and "Asynchronous Redirection" make sense to me)

@lucavenir
Copy link

lucavenir commented Oct 5, 2022

Hi @creativecreatorormaybenot

On Riverpod's documentation pages, under the 3rd party examples folder on the left, you'll find "GoRouter with Riverpod", which is a repo I've made a while ago you can explore to check out possible solutions to integrate GoRouter and Riverpod together.

I also want to add that while "This setup is so much cleaner and easier to grasp than the existing Redirection example using provider", there are some (simple) caveats to be aware of.

In your example snippet above, routerProvider will rebuild everytime there's an authentication change; this will most likely cause a whole app rebuild (onto the root of our apps), since I'd expect the main App to watch our routerProvider. This is most likely undesired.

The above (and more caveats) are what motivated this repo to not only write down a simple example, but to properly analyze different possible approaches.

Indeed, very soon I am going to add (there) the following new features:

  • Integration with GoRouter v5 (and its asynchronous redirect once this issue is fixed)
  • Usage with Riverpod v2's new features (where it makes sense)
  • Usage with a Listenable (instead of a ChangeNotifier)
  • Writing of Tests about the aforementioned examples

Feedback is highly appreciated.

@creativecreatorormaybenot
Copy link
Contributor Author

Do you plan to add this example yourself? If not, I can take this.

@TahaTesser That would be super awesome! I certainly think that having Riverpod examples makes sense 👍

I would be glad to review 👌

@creativecreatorormaybenot
Copy link
Contributor Author

creativecreatorormaybenot commented Oct 5, 2022

In your example snippet above, routerProvider will rebuild everytime there's an authentication change; this will most likely cause a whole app rebuild (onto the root of our apps), since I'd expect the main App to watch our routerProvider. This is most likely undesired.

Hi @lucavenir, this is not right. If you look at the snippet, you will see that I consciously pass a navigatorKey to GoRouter. This means that the tree will be kept upon "rebuilding" the routerProvider. This makes use of Flutter's ability to keep the tree by comparing its elements. Because the setup is the exact same before and after, the same widgets will be inserted and if you write your widgets correctly, all state will be kept and nothing will be discarded.

The above (and more caveats) are what motivated this repo to not only write down a simple example, but to properly analyze different possible approaches.

I very highly appreciate this effort 🙏 Thanks @lucavenir. However, I am sorry to tell you that I do not like the solutions in this repo. Especially piping all provider changes into listenables seems awful to maintain.

Feel free to try out my proposed solution and report if you can find any problems with it. This is also why I would really love to have an example/ for this setup in go_router because we could properly test if there are any problems with state being discarded or undesired rebuilds.

About rebuilds

Note that you are of course right that the App in this setup:

class App extends ConsumerWidget {
  const App({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final router = ref.watch(routerProvider);
    return MaterialApp.router(
      routerConfig: router,
    );
  }
}

Will always rebuild when one of the dependencies of the routerProvider changes. However, since we pass the navigatorKey, we really do not care about this. This rebuild basically has minimal cost (and the framework could also trigger it at any time) and all of our actual tree, i.e. our pages returned from the router, that we may not want to rebuild unnecessarily will not rebuild. That is because the framework checks if the pages have updated and they will not update unless there is an actual change in navigation.

@creativecreatorormaybenot
Copy link
Contributor Author

creativecreatorormaybenot commented Oct 5, 2022

One crazy idea I have is removing refreshListenable altogether and recommending to rebuild GoRouter instead. Say using ValueListenableBuilder or using flutter_riverpod as outlined. To me this seems so much cleaner and would also reduce complexity.

If we further optimize GoRouter for being reconstructed, this would be the perfect solution for redirection 🚀 And it fits the declarative nature of go_router better ✨

@lucavenir
Copy link

lucavenir commented Oct 5, 2022

Hi @lucavenir, this is not right. If you look at the snippet, you will see that I consciously pass a navigatorKey to GoRouter. This means that the tree will be kept upon "rebuilding" the routerProvider. This makes use of Flutter's ability to keep the tree by comparing its elements. Because the setup is the exact same before and after, the same widgets will be inserted and if you write your widgets correctly, all state will be kept and nothing will be discarded.
Feel free to try out my proposed solution and report if you can find any problems with it.

I've been sloppy in reading your code - sorry! Thanks for the detailed explanation. I'll test this out better later, as I can't seem to avoid rebuilds with just that NavigatorKey.
If this - in the end - works out fine, I'm thinking about redesigning a lot of the examples I've wrote in that repo: we could just use Providers like we always do: we watch elements in there declaratively, without implementing external Listenables. I have to test this out and evaluate its scalability and if it's feasible (assuming the Flutter tree won't rebuild - our Provider containing the Router still does).

Especially piping all provider changes into listenables seems awful to maintain.

I'm obsessed with best practices and I can't agree more! The repo looks like this (atm) because:

  1. It first aimed at trying solutions proposed by the community, as this topic is very opinionated (see this discussion)
  2. I am now re-writing most of the examples towards using newer API (GoRouter v5, Riverpod v2)
  3. I am planning to erase out all the "not-so-good" practices in some examples. I think I'll keep just the "good ones" and I'll erase (what feels like) legacy code
  4. Given that your solution works, until now we didn't find a better scalable and maintainable alternative. But I can see the light with GoRouter v5 + Riverpod v2!
  5. The goal is to maintain that repo as a "community wiki" for Riverpod (after this discussion, there's a good chance I'll touch the examples even more in the next days).

If those examples (once corrected) contribute into a "Flutter-recommended" approach, it would be awesome and it would answer a lot of questions arising from the Riverpod's community.

This is also why I would really love to have an example/ for this setup in go_router

I absolutely agree with this. See the above sentences.

@lucavenir
Copy link

@creativecreatorormaybenot Do you mind take a look at this branch? I'm trying your suggestion but I still seeing the "root widget" rebuilding, there.

@creativecreatorormaybenot
Copy link
Contributor Author

creativecreatorormaybenot commented Oct 5, 2022

I am not sure what the "root widget" in your setup means @lucavenir, but here you go: https://dartpad.dev/0a77fd6105aeee44858b794e17e83ffc
This is a full setup using my riverpod + go_router approach.

Here is an in-depth showcase 💯 Here we have (sound on 🔈)..

Screen.Recording.2022-10-05.at.22.19.27.mp4

Note that the MaterialApp itself also does not rebuild internally because it does not change. So it is mainly the App.build call, which is very cheap.

@lucavenir
Copy link

Thank you @creativecreatorormaybenot for the in-depth explanation!! This is way clearer. I guess I'm still confused by Flutter's keys.

@lucavenir
Copy link

Hi @creativecreatorormaybenot !

I've updated the examples in this repo, thanks to you! I've cited you as a contributor, but I'm unsure on how to give you more credit than that 😛

Your feedback would be highly appreciated - and I'm guessing that the complete_example folder in there could be used to solve this issue 👀

@stuartmorgan stuartmorgan added the P3 Issues that are less important to the Flutter project label Oct 10, 2022
@creativecreatorormaybenot
Copy link
Contributor Author

I really appreciate the sample @lucavenir. The crediting is fine. I think a link to the issue would help for context 🙂

Note that I discovered a problem. In the case that you do not cover all of your routes in your redirect handler (e.g. if you have nested routes and only redirect to and from the top route), rebuilding the router will mess up your current state as the initial location is always /.

The workaround for this is adding:

...
// We need to have access to the previous location of the router. Otherwise, we would start from '/' on rebuild.
GoRouter? _previousRouter;

final routerProvider = Provider((ref) {
  return GoRouter(
    initialLocation: _previousRouter?.location,
    ...,
  );
});

I am not the biggest fan of this since it is even less clean (with the router being outside of any providers). However, it does work and is solid / non-fragile in theory.

Another implication is that modals will never work with this approach, i.e. routes that are pushed via GoRouter.of(context).push since they are not encapsulated in the current location. What does work is modals inside of a page, e.g. modal_bottom_sheet. So this is not an issue.
For routes that are pushed on top of the whole stack / not part of the routes defined in routes, I have come up with my own solution that integrates way better with riverpod anyway.

That is to say, I still greatly prefer this method over the previous methods. Although, we should be aware of the limitations.


I really like your samples - good job! They are not complex enough to have encountered the edge cases I mentioned, so it might be worth playing with it a bit more to discover these as well 👍

@Neelansh-ns
Copy link

I am not sure what the "root widget" in your setup means @lucavenir, but here you go: https://dartpad.dev/0a77fd6105aeee44858b794e17e83ffc This is a full setup using my riverpod + go_router approach.

Here is an in-depth showcase 💯 Here we have (sound on 🔈)..

Screen.Recording.2022-10-05.at.22.19.27.mp4
Note that the MaterialApp itself also does not rebuild internally because it does not change. So it is mainly the App.build call, which is very cheap.

This solution is not working as shown in the video on the latest stable channel 3.7.3. @creativecreatorormaybenot

@creativecreatorormaybenot
Copy link
Contributor Author

It may be that you skipped one of the steps I described @Neelansh-ns. I just tested the Gist at https://dartpad.dev/0a77fd6105aeee44858b794e17e83ffc and it works for me.

@lucavenir
Copy link

lucavenir commented Feb 13, 2023

Hi there,

I want to share some thoughts.

  1. I feel this example is very much needed. I'm still pretty much proud (😂) of the examples from my repo, but maybe having an "official" go-to solution is best;
  2. After some trials, and after riverpod v2, together with riverpod_generator, I like this solution better. In a nutshell: there's no passing of a ref object, router isn't rebuilding on authentication changes and our notifier actually implements Listenable, which is what's required by current's go_router's API.
  3. All of this conversation might be overridden by this comment from go_router maintainers. I have a gut feeling that there's a good chance this issue is being "delayed" for a good reason: go_router is still on its way of a "best" design;
  4. Therefore, taking all of this into account (i.e. go_router APIs have changed in a way that is making the maintainers think refreshListenable might be deprecated) and adding this other comment to the equation, maybe this solution might be a good reference. Personal note: I actually don't like this last solution; I don't find it declarative and I don't like the fact the responsibility of triggering redirects is moved away from router.

@rirjkl19
Copy link

rirjkl19 commented Jun 3, 2023

@creativecreatorormaybenot Would like to ask on this. I actually liked your provided solution as everything make sense in first look. However,

I have come up with my own solution that integrates way better with riverpod anyway.

What was your solution on handling modals on top of the route stack?

And as for this. How do you update the _previousRouter then?

...
// We need to have access to the previous location of the router. Otherwise, we would start from '/' on rebuild.
GoRouter? _previousRouter;

final routerProvider = Provider((ref) {
  return GoRouter(
    initialLocation: _previousRouter?.location,
    ...,
  );
});

@julgog
Copy link

julgog commented Oct 4, 2023

creativecreatorormaybenot
@creativecreatorormaybenot hello , sorry but your example only works for the web platform, testing on Android the widgets are rebuilt despite having the navigatorkey.
I think that even though it is rebuilt, the changes in authentication occur at startup and when closing, so in the worst case it would only be rebuilt twice while using the application.
Anyway I would like a cleaner solution if anyone has an idea how to use go_router with firebase

@mvarendorff
Copy link

A note on _previousRouter?.location; since GoRouter 9.0.0, it should be _previousRouter?.routerDelegate.currentConfiguration.fullPath.

@CesareIsHere
Copy link

Hi there,

I solved this problem by creating a router provider that who takes care about update the router every time the auth state changes.

I did some tests but i'm not sure if is conceptually right or not, so I would appreciate it if you could give me some feedback.

RouterNotifier:

@riverpod
class RouterNotifier extends _$RouterNotifier {
  @override
  FutureOr<GoRouter> build(WidgetRef reff) {
    ref.watch(authNotifierProvider);
    return routerGenerator(
      ref: reff,
      rootNavigatorKey: rootNavigatorKey,
      shellNavigatorKey: shellNavigatorKey,
    );
  }
}

In the MaterialApp i have just added the watcher to the gorouter notifier value

MaterialApp.router(
        title: 'App',
        debugShowCheckedModeBanner: false,
        routerConfig: ref.watch(routerNotifierProvider(ref)).value,
      )

Than this is the function that create the GoRouter object

GoRouter routerGenerator({
  required WidgetRef ref,
  GlobalKey<NavigatorState>? rootNavigatorKey,
  GlobalKey<NavigatorState>? shellNavigatorKey,
}) {
  return GoRouter(
    navigatorKey: rootNavigatorKey,
    initialLocation: PostsPage.route,
    redirect: (context, state) {
      if (!ref.read(authNotifierProvider.notifier).isSignedIn()) {
        return LoginPage.route;
      }
      return null;
    },
    routes: ...

Thank you for your feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: proposal A detailed proposal for a change to Flutter d: examples Sample code and demos p: go_router The go_router package P3 Issues that are less important to the Flutter project package flutter/packages repository. See also p: labels. team-go_router Owned by Go Router team triaged-go_router Triaged by Go Router team
Projects
No open projects
Status: No status
Development

No branches or pull requests