csells / go_router

The purpose of the go_router for Flutter is to use declarative routes to reduce complexity, regardless of the platform you're targeting (mobile, web, desktop), handling deep linking from Android, iOS and the web while still allowing an easy-to-use developer experience.
https://gorouter.dev
441 stars 97 forks source link

Bad path and route name error handling #322

Closed walsha2 closed 2 years ago

walsha2 commented 2 years ago

Description

@csells I have been banging my head against this and I do not get it. It might be an unexpected bug or I am not understanding how errorBuilder is supposed to work? In summary, I cannot to get errorBuilder to trigger whenever this is any navigation error, only in certain circumstances.

Please see the example below. The first two examples are strings for bad paths or bad page names. None of these will trigger the error builder. Instead I get uncaught exceptions. My expectation was that the errorBuilder would catch any navigation issues. Since paths and names are simply strings, it is entirely possible and expected that there may be an unexpected routing issue at some point during development or heck maybe even production. I would like to be able to catch all of these navigation exceptions and basically route to the web equivalent 404 error, i.e. errorBuilder.

Am I not understanding something and is this expected behavior? How else does one catch bad route names or bad route paths?

The second example does not work altogether.. This is the one I am most surprised by. There is no way to handle bad route names from what I can see.

Side Note - After the Fact

I realized something while writing this issue and these assert statements for the first example. The first example does actually work as expected, but only in release mode.

I am not the biggest fan of these assert statements in GoRouteMatch. Can't these routes be routed to the error builder instead? I would prefer that the user cue to fix something be an unexpected navigation to the error page, as opposed to an assertion error. It took me quite a bit to realize that if I ran in release mode the behavior would actually be different and the errorBuilder would indeed trigger for the first example. This could easily be a Logger warning as opposed to an assertion, no?

Again, my big thing here is that paths and names are strings and we can/should expect errors. They should be handled by the errorBuilder and not produce assertion errors (example 1) or uncaught errors (example 2).

assert(subloc.startsWith('/')),
assert(fullpath.startsWith('/')),

Minimal Example

void main() => runApp(App());

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

  static const title = 'GoRouter Example: Custom Error Screen';

  @override
  Widget build(BuildContext context) => MaterialApp.router(
        routeInformationParser: _router.routeInformationParser,
        routerDelegate: _router.routerDelegate,
        title: title,
      );

  final _router = GoRouter(
    routes: [
      GoRoute(
        path: '/',
        builder: (context, state) => const Page1Screen(),
      ),
      GoRoute(
        path: '/page2',
        builder: (context, state) => const Page2Screen(),
      ),
    ],
    errorBuilder: (context, state) => ErrorScreen(state.error!),
  );
}

class Page1Screen extends StatelessWidget {
  const Page1Screen({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) => Scaffold(
        appBar: AppBar(title: const Text(App.title)),
        body: Center(
          child: Column(
            mainAxisAlignment: MainAxisAlignment.center,
            children: [
              ElevatedButton(
                onPressed: () => context.go('badPagePath'),
                child: const Text("Doesn't Work: go('badPagePath')"),
              ),
              ElevatedButton(
                onPressed: () => context.goNamed('badPageName'),
                child: const Text("Doesn't Work: goNamed('badPageName')"),
              ),
              ElevatedButton(
                onPressed: () => context.go('/badPagePath'),
                child: const Text("Works: go('/badPagePath')"),
              ),
            ],
          ),
        ),
      );
}

class Page2Screen extends StatelessWidget {
  const Page2Screen({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) => Scaffold(
        appBar: AppBar(title: const Text(App.title)),
        body: Center(
          child: Column(
            mainAxisAlignment: MainAxisAlignment.center,
            children: [
              ElevatedButton(
                onPressed: () => context.go('/'),
                child: const Text('Go to home page'),
              ),
            ],
          ),
        ),
      );
}

class ErrorScreen extends StatelessWidget {
  const ErrorScreen(this.error, {Key? key}) : super(key: key);
  final Exception error;

  @override
  Widget build(BuildContext context) => Scaffold(
        appBar: AppBar(title: const Text('My "Page Not Found" Screen')),
        body: Center(
          child: Column(
            mainAxisAlignment: MainAxisAlignment.center,
            children: [
              SelectableText(error.toString()),
              TextButton(
                onPressed: () => context.go('/'),
                child: const Text('Home'),
              ),
            ],
          ),
        ),
      );
}
csells commented 2 years ago

This is all working as expected. It's possible for the user to enter a bad path via the browser's address bar and in those cases, an error page will be shown. However, if there are errors that only a developer can make, e.g. not starting a location with a leading '/' or going to a named route that doesn't exist, then those errors are turned into exceptions. The idea is to make the dev-related errors loud and proud so that the app can be fixed and saving the error page for the user-related data entry errors.

walsha2 commented 2 years ago

Understood. Thanks!