routemaster icon indicating copy to clipboard operation
routemaster copied to clipboard

Async routes or guards

Open tomgilder opened this issue 3 years ago • 23 comments

There are some use cases where being able to use async within route builders would be useful.

But this adds complication and isn't as easy as it might sound.

Will be looked into after a stable 1.0.

tomgilder avatar Apr 06 '21 19:04 tomgilder

i believe awaiting pop results is a generally popular use case

theweiweiway avatar Apr 26 '21 23:04 theweiweiway

Yeah, that's an interesting one. Currently Routemaster doesn't support pop results at all, and I'm not sure if it's easy to add them - because a push isn't a simple push of just one page in the stack, it can go anywhere.

tomgilder avatar Apr 27 '21 06:04 tomgilder

Guards definitely make sense if you would like to perform validation inside of the guard.

chimon2000 avatar Apr 27 '21 17:04 chimon2000

I agree that it feels like guards should allow async - but I'm not sure how this should work in practice.

Whilst the async method is running, what should happen?

Should the router lock and prevent other navigation?

What's the end use scenario here?

tomgilder avatar Apr 27 '21 19:04 tomgilder

Here's the scenario that I find hard to figure out:

  1. You have a Guard that takes an async method that takes 3 seconds to return
  2. User navigates to that URL
  3. What does the user see for those 3 seconds? What if they try to cancel the navigation?

But at the time, there are async methods that are quick and do I/O, which absolutely should be supported.

What about having an AsyncGuard that allowed you to show another page or spinner whilst it's loading?

tomgilder avatar Apr 28 '21 06:04 tomgilder

Something unrelated to this issue - but related to guards - is a redirectTo variable. Sometimes when you get redirected by a guard, you want to complete the guard and then redirect back to the initial page you were trying to get into. Usually on web - we can do this with a queryParameter. Not sure if on Flutter we can do a more integrated approach?

And in Flutter, it would probably be "popping" all the way back to the original page you were trying to access? Maybe there could be a flag like so:

Guard(
  redirect: true
)

Not sure how that would work tho..

theweiweiway avatar Apr 28 '21 13:04 theweiweiway

Here's the scenario that I find hard to figure out:

  1. You have a Guard that takes an async method that takes 3 seconds to return
  2. User navigates to that URL
  3. What does the user see for those 3 seconds? What if they try to cancel the navigation?

But at the time, there are async methods that are quick and do I/O, which absolutely should be supported.

What about having an AsyncGuard that allowed you to show another page or spinner whilst it's loading?

I like the idea of having an AsyncGuard, although I'm not sure it's the responsibility of the library to display loading in this scenario. Using the Angular guard as the most flexible example:

interface CanActivate {
  canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean | UrlTree> | Promise<boolean | UrlTree> | boolean | UrlTree
}

Just focusing on the first two scenarios since this is about async.

  1. The developer returns an observable (stream) to show some route/UI while loading data and then replace that with some other route/UI.
  2. The developer returns a promise (future), blocking while data is loading then replacing that with some route/UI

I can see both as useful, maybe swapping out the stream for a ValueListenable, or just keeping it as is.

The Flutter examples that I have seen (vrouter, flutter_modular, qlevar_router) all handle the 2nd scenario.

chimon2000 avatar Apr 28 '21 16:04 chimon2000

I'm not sure it's the responsibility of the library to display loading

Absolutely, I'd never add this to the library - it'll be up to the user to show a loading spinner/page.

The developer returns an observable (stream) to show some route/UI while loading data and then replace that with some other route/UI.

Interesting - how about an async method that can return multiple pages?

So you'd end up with something like this:

Stream<Page> buildRoute() async* {
  yield MaterialPage<void>(child: LoadingPage());

  if (await canShowPage()) {
    yield MaterialPage<void>(child: ContentPage());
  } else {
    yield MaterialPage<void>(child: BlockedPage());
  }
}

tomgilder avatar Apr 28 '21 19:04 tomgilder

I'm not sure it's the responsibility of the library to display loading

Absolutely, I'd never add this to the library - it'll be up to the user to show a loading spinner/page.

The developer returns an observable (stream) to show some route/UI while loading data and then replace that with some other route/UI.

Interesting - how about an async method that can return multiple pages?

So you'd end up with something like this:

Stream<Page> buildRoute() async* {
  yield MaterialPage<void>(child: LoadingPage());

  if (await canShowPage()) {
    yield MaterialPage<void>(child: ContentPage());
  } else {
    yield MaterialPage<void>(child: BlockedPage());
  }
}

Yes, I think this is potentially a very clean solution! Slightly tweaking to display redirects.

Stream<Page> buildRoute() async* {
  yield MaterialPage<void>(child: LoadingPage());

  if (await canShowPage()) {
    yield MaterialPage<void>(child: ContentPage());
  } else {
    yield Redirect('/login');
  }
}

chimon2000 avatar Apr 29 '21 13:04 chimon2000

Generally I'm heading towards thinking async in the router itself is a bad idea.

This is for the same reason Flutter doesn't allow build methods to be async - it needs to always know what to render.

The above example can be done within the Page currently:

'/async': (_) => MaterialPage<void>(
      child: StreamWidgetBuilder(
        builder: (context) async* {
          yield Scaffold(body: Center(child: Text('Loading...')));

          if (await canShowPage()) {
            yield Scaffold(body: Center(child: Text('Success')));
          } else {
            Routemaster.of(context).replace('/');
          }
        },
      ),
    ),

Source for StreamWidgetBuilder:

StreamWidgetBuilder
class StreamWidgetBuilder extends StatefulWidget {
  final Stream<Widget> Function(BuildContext) builder;
  final Widget initialChild;

  const StreamWidgetBuilder({
    Key? key,
    required this.builder,
    this.initialChild = const SizedBox(),
  }) : super(key: key);

  @override
  _StreamWidgetBuilderState createState() => _StreamWidgetBuilderState();
}

class _StreamWidgetBuilderState extends State<StreamWidgetBuilder> {
  late StreamSubscription<Widget> _subscription;
  late Widget _child;

  @override
  void initState() {
    super.initState();

    _child = widget.initialChild;
    _subscription = widget.builder(context).listen((newChild) {
      setState(() => _child = newChild);
    });
  }

  @override
  void dispose() {
    _subscription.cancel();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) => _child;
}

But I'd love any specific scenarios that would be unachievable without async.

tomgilder avatar May 02 '21 09:05 tomgilder

But I'd love any specific scenarios that would be unachievable without async.

Sure: basically any authorization/authentication call to a remote data source (API) or a local data source (shared preferences). These are all technically achievable using StreamBuilders and FutureBuilders.

I think there's not a huge difference between your initial proposal of the AsyncGuard. Most of the guards that I would depend on will normally depend on an API call, so I would probably have to fallback to using builders and just not use guards, or use them solely for static information.

The above proposal works although the idea of calling a function in a build function feels like a code smell. Might be better to have a Redirect widget that accomplishes the same thing in initState.

chimon2000 avatar May 03 '21 02:05 chimon2000

The above proposal works although the idea of calling a function in a build function feels like a code smell. Might be better to have a Redirect widget that accomplishes the same thing in initState.

Great idea. But at the moment I've defined Redirect as a Page to use in the routing table 🤦 Argh!

And if only union types were a thing, it would make this so much simpler - I'd love to make a routing table where you can return either a Widget or Page and it just works.

tomgilder avatar May 03 '21 05:05 tomgilder

Continuing your thought process on https://github.com/tomgilder/routemaster/issues/19#issuecomment-831226973, I really like the idea of deprecating guards in favor of just being able to return a Redirect or MaterialPage when declaring routes.

'/protected-route': (route) {
    if (!isLoggedIn()) return Redirect('/login');
    if (!canUserAccessPage) return Redirect('/no-access');
    return ProtectedPage();
},

In a more complex example, I could see myself wrapping an app in a bootstrap widget that would load authentication/authorization logic using a FutureBuilder/StreamBuilderbefore the routes are instantiated.

chimon2000 avatar May 04 '21 23:05 chimon2000

For now, I'm not going to implement any async behaviour. I want to get to a stable 1.0 release without the complexity of async.

I'll revisit this after that.

tomgilder avatar May 06 '21 05:05 tomgilder

I've added a new example that shows how to handle several common async scenarios: logging in and deep linking.

See here: https://github.com/tomgilder/routemaster/tree/main/example/deep_linking

tomgilder avatar May 07 '21 08:05 tomgilder

Another scenario, pretty similar to login one, but subject to more changes during runtime.

Let's say we have a context, a house for example, urls could be like /house/1/edit. Navigating to /house/2/edit could imply a reload of the house context:

  • data/logic: related permission vs. current user to edit, view or whatever.
  • UI: changing theme, etc.

This could lead to multiple unwanted checks in all sub-pages.

rnl-veolys avatar Jun 10 '21 08:06 rnl-veolys

I've made an initial working prototype of this.

Here's an example of the current API:

'/async-page': (_) => AsyncPageBuilder(
      initialPage: MaterialPage(child: LoadingPage()),
      pageBuilder: () async* {
        final isLoggedIn = await appState.isLoggedIn();

        if (isLoggedIn) {
          yield Redirect('/login-page');
        } else {
          yield MaterialPage(child: HomePage());
        }
      },
    ),

It currently requires an initialPage. This could just be a totally blank page.

It might be possible to avoid having an initialPage, and just wait until the first page is yielded from the pageBuilder.

If there is a Routemaster navigation event (such as the user entering a new URL), any further yielded pages are ignored to avoid confusing of background pages suddenly redirecting somewhere else.

Feedback welcome!

tomgilder avatar Aug 02 '21 09:08 tomgilder

Okay, yeah, I found a way to get rid of initialPage... so it now works like this:

'/async-page': (_) => AsyncPageBuilder(
      pageBuilder: () async* {
        yield MaterialPage(child: LoadingPage());
        final isLoggedIn = await appState.isLoggedIn();

        if (isLoggedIn) {
          yield Redirect('/login-page');
        } else {
          yield MaterialPage(child: HomePage());
        }
      },
    ),

Only one API question left: does the async* method need a way to know that the subscription has been cancelled? As in, the user has navigated to a new route? If so, how does it find out?

tomgilder avatar Aug 02 '21 09:08 tomgilder

Seems great! About cancellation, I don't think we need to know about. This would result in bloated code anyway IMHO.

When cancelled, pageBuilder block is still fully processed (or am I wrong?), widget loaded but not attached to view hierarchy. So there should be no cascaded data loading.

What's the behaviour if nothing is generated? Expecting no route change here.

rnl-veolys avatar Aug 03 '21 09:08 rnl-veolys

About cancellation, I don't think we need to know about.

I think there are a handful of cases where this is useful.

I'm currently thinking of just adding a parameter to the method:

'/async-page': (_) => AsyncPageBuilder(
      pageBuilder: (status) async* {
        yield MaterialPage(child: LoadingPage());
        final isLoggedIn = await appState.isLoggedIn();

        if (status.isCancelled) return;
        await doSomethingExpensive();
        if (status.isCancelled) return;
        await doAPICall();
        ...
      },
    ),

What's the behaviour if nothing is generated? Expecting no route change here.

Good point. Currently it would do a navigation, but nothing would appear. A totally empty invisible page is added to the navigation stack, but the URL changes immediately. I agree it would be better if nothing happened at all; I'll look into it.

tomgilder avatar Aug 03 '21 10:08 tomgilder

This would open the door to bad conceptions. What if doAPICall throws an exception? Devs cannot recover the error from here, user is not informed, etc.

All the UI guidelines lead to provide the loading time period in the page itself with dedicated widget or variants of this. We're navigating here, I vote for enforcing it to throw away as far as possible business cases.

rnl-veolys avatar Aug 03 '21 11:08 rnl-veolys

What about making a guard just async function returning Future and exposing some navigation state via Routemaster.of(context).state? The navigation state could contain some useful information about the current route navigating such as

from where to where isGuarding --> If the guard is an async function, the navigating would be this status isTransitioning

This will allow the app to listen to navigation state changes and update the UI accordingly. It would be easy to implement the top loading bar. It seems it's not possible to implement top loading bar with the above async guard async* function.

I don't know if this is technically possible but just wanted to drop some of the ideas and suggesstions.

jezsung avatar Sep 17 '21 15:09 jezsung

Here is an example of an easy way to create an async page that waits for a future to finish before loading the desired screen. It also handles displaying a loading screen while the future is running, and an error screen on error.

Use Case: in my case, for Flutter web, a user can enter the app from any internal screen and all screens require authentication. I would use a future that attempts to restore the user from persistent storage, and refresh their authentication tokens, before loading the desired screen. If the user does not exist or is expired, then a redirect can be handled with additional code.

AsyncMaterialPage()

/// Async Material Page
class AsyncMaterialPage extends MaterialPage {
  final Widget screen;
  final Future future;
  AsyncMaterialPage({required this.screen, required this.future})
      : super(
          child: FutureBuilder(
            future: future,
            builder: (context, snapshot) {
              if (snapshot.hasError) {
                print("App Init Failure: " + snapshot.error.toString());
                return const AppInitFailScreen();
              }

              // When initAppFeatures future is done, load the main app Widget
              if (snapshot.connectionState == ConnectionState.done) {
                return screen;
              }

              // Otherwise return loader
              return const AppInitLoadingScreen();
            },
          ),
        );
}

An example of a RouteMap() where you can test a Future.delayed, and a Future.error scenario. Replaceable with any function that returns a future.

  return RouteMap(
    onUnknownRoute: (_) => const Redirect('/'),
    routes: {
      // '/' Initial route uses InitialPageBuilder() to complete app initialization functions at startup
      '/': (_) => AsyncMaterialPage(
            future: Future.delayed((Duration(seconds: 5))),
            // future: Future.error("Error future"),
            screen: LoginScreen(),
          ),
    },
  );

If your future will always be the same (like you have an app startup process that will need to potentially be run every time the app starts for any page visited - which can happen from Web), then you could omit the future: field from AsyncMaterialPage(), and just wire the future directly into the FutureBuilder method in the constructor.

Dynamic RouteMap With Riverpod

In my case, the standard AppStartup future will make the global Riverpod state for the user state either null, or have a user object.

I can create a routeMapProvider to watch the user state and return the loggedOut routeMap or loggedIn routeMap dynamically. Link to Riverpod docs about providers watching other providers to dynamically update on state changes.

(Here, the appInitComplete state is also watched, so that we only call this process once at app startup, and don't trigger an infinite loop. The app startup future checks to only run if it's false. We also don't want to return the logged out route map before we fetch the user because we don't want to redirect them away from the internal URL that may not exist on our smaller loggedOut user routemap)

final routeMapProvider = Provider<RouteMap>((ref) {
  final User? _user = ref.watch(userObjectOnlyProvider);
  final bool _appInitComplete = ref.watch(globalStateProvider).appInitComplete;
  logger.d("Rebuilding root map. Init complete? $_appInitComplete. User null? " + (_user == null).toString());

  if (_user == null && _appInitComplete == true) {
    return RouteMap(
        // logged out route map here
    )
  }
  return RouteMap(
    // logged in route map here
  )
}

Here is an example of a root MaterialApp.router() widget that listens to the riverpod provider, to change the routesBuilder: RouteMap dynamically as the user state changes.

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

  @override
  Widget build(BuildContext context, ScopedReader watch) {
    logger.d("Building MaterialApp.router");
    return FeatureDiscovery(
      child: MaterialApp.router(
        locale: context.locale,
        supportedLocales: context.supportedLocales,
        localizationsDelegates: context.localizationDelegates,
        debugShowCheckedModeBanner: false,
        scaffoldMessengerKey: rootScaffoldMessengerKey,
        title: 'MyApp',
        theme: MyAppTheme.themeData(context),
        routerDelegate: RoutemasterDelegate(
          observers: [ConnectRouteObserver()],
          routesBuilder: (context) {
            // Watch for user state changes to rebuild our RouteMap based on user login state
            RouteMap _routeMap = watch(routeMapProvider);
            return _routeMap;
          },
        ),
        routeInformationParser: const RoutemasterParser(),
      ),
    );
  }
}

mdrideout avatar Sep 17 '21 20:09 mdrideout