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
442 stars 96 forks source link

Parameters are not decoded when retrieved. #120

Closed kevindmoore closed 2 years ago

kevindmoore commented 2 years ago

If you pass strings as parameters that contain spaces, the spaces in the string gets encoded into %20 and not decoded on the way back.

Passing parameter:

context.pushNamed(detailsPageRouteName, params: {item: value});

Receiving:

      GoRoute(
        name: detailsPageRouteName,
        path: detailsPageRoute,
        pageBuilder: (context, state) => MaterialPage<void>(
          key: state.pageKey,
          child: Builder(
            builder: (context) {
              return Details(description: Uri.decodeComponent(state.params[item]!));
            },
          ),
        ),
      ),

I needed to add the Uri.decodeComponent to get it to work

csells commented 2 years ago

@kevindmoore I've got the test to repro the issue. working on a fix now.

csells commented 2 years ago

fixed in v2.2.2. @kevindmoore can you confirm?

kevindmoore commented 2 years ago

I'm using v2.2.4 and I still see the issue

csells commented 2 years ago

can you post a minimal repro project? it's working in my tests.

kevindmoore commented 2 years ago

Here is what I have on the 1 screen:

            onTap: () {
              final value = items[index];
              context.pushNamed(detailsPageRouteName, params: {item: value});
            },

where items is:

    final items = List<String>.generate(10000, (i) => 'Item $i');

Then my route is defined as:

     GoRoute(
        name: detailsPageRouteName,
        path: detailsPageRoute,
        pageBuilder: (context, state) => MaterialPage<void>(
          key: state.pageKey,
          child: Builder(
            builder: (context) {
              return Details(
                  description: Uri.decodeComponent(state.params[item]!));
            },
          ),
        ),

with constants:

const String detailsPageRouteName = '/details';
const String detailsPageRoute = '/details/:item';

Details is:

class Details extends StatelessWidget {
  final String description;

  const Details({Key? key, required this.description}) : super(key: key);
}
csells commented 2 years ago

@kevindmoore here's a minimal repro based on your snippets (I needed a fully running app to debug):

import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';

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

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

  @override
  Widget build(BuildContext context) => MaterialApp.router(
        routeInformationParser: _router.routeInformationParser,
        routerDelegate: _router.routerDelegate,
        title: "GoRouter Example: Kevin's issue #120 repro",
      );

  late final _router = GoRouter(
    debugLogDiagnostics: true,
    routes: [
      GoRoute(
        path: '/',
        redirect: (_) => '/details',
      ),
      GoRoute(
        name: 'items',
        path: '/details',
        pageBuilder: (context, state) => MaterialPage<void>(
          key: state.pageKey,
          child: ItemsPage(),
        ),
      ),
      GoRoute(
        name: 'details',
        path: '/details/:item',
        pageBuilder: (context, state) => MaterialPage<void>(
          key: state.pageKey,
          child: DetailsPage(description: state.params['item']!), // ERROR!
        ),
      ),
    ],
    errorPageBuilder: (context, state) => MaterialPage<void>(
      key: state.pageKey,
      child: ErrorPage(state.error),
    ),
  );
}

class ItemsPage extends StatelessWidget {
  ItemsPage({Key? key}) : super(key: key);
  final items = List<String>.generate(10, (i) => 'Item $i');

  @override
  Widget build(BuildContext context) => Scaffold(
        body: ListView(children: [
          for (var index = 0; index != items.length; ++index)
            ListTile(
              title: Text(items[index]),
              onTap: () => context.pushNamed(
                'details',
                params: {'item': items[index]},
              ),
            )
        ]),
      );
}

class DetailsPage extends StatelessWidget {
  const DetailsPage({required this.description, Key? key}) : super(key: key);
  final String description;

  @override
  Widget build(BuildContext context) => Scaffold(body: Text(description));
}

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

  @override
  Widget build(BuildContext context) => Scaffold(
        appBar: AppBar(title: const Text('Page Not Found')),
        body: Center(
          child: Column(
            mainAxisAlignment: MainAxisAlignment.center,
            children: [
              Text(error?.toString() ?? 'page not found'),
              TextButton(
                onPressed: () => context.go('/'),
                child: const Text('Home'),
              ),
            ],
          ),
        ),
      );
}

A couple of notes:

  1. I wouldn't put slashes into your names; I've made the name for the DetailsPage route just details instead of /details. Names are just for looking up routes, not actually building them (that's what the path is for). It works either way but I find including slashes into the name is confusing.
  2. I wouldn't use push() or pushNamed() to build a stack; instead, I'd make the details route a sub-route of the "items" page (I had to guess at a name here, since you didn't provide one in you snippets) and use go() or goNamed() to build up the stack of pages. Check out the sub-routes docs for details.

I was able to duplicate your bug the repro above and then found the error in my tests that hid the bug (don't you hate that?!). I've fixed my tests, repro'd your bug and will be publishing a fix momentarily. Sorry for the delay. Keep those cards and letters coming!

csells commented 2 years ago

fixed in v2.2.5 (I think). @kevindmoore can you confirm?