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

Query parameters persist after navigation #170

Closed benPesso closed 2 years ago

benPesso commented 2 years ago

Given some logic in the router's redirect handler that redirects the app to a path with some query parameters in it (E.g.: /view-invoice?url=$invoiceUrl)... Once the user navigates back from that route (via back swipe gesture / back button), the state.queryParams object still holds the parameters from the view-invoice page, even though the user was taken to a new route with no parameters.

Example flow:

  1. Navigate to /.
  2. Navigate to /view-invoice?url=$invoiceUrl.
  3. Navigate to /. <-- When printing the state.queryParams in the redirect handler, the object still holds the old values.

I'm not sure if this is by design or not, but if it is, there should at least be a way to clear the params manually. Maybe via an onBackNavigation handler?

csells commented 2 years ago

what do your routes look like?

benPesso commented 2 years ago

The router looks something like this:

GoRouter(
        refreshListenable: ref.read(incomingInvoiceProvider.notifier), // A change notifier
        redirect: _globalRedirect,
        routes: [
          GoRoute(
              name: home.name,
              path: home.path,
              pageBuilder: (BuildContext context, GoRouterState state) =>
                  MaterialPage<void>(
                    key: state.pageKey,
                    child: const HomePage(),
                  ),
              routes: [
                GoRoute(
                  name: viewInvoice.name,
                  path: viewInvoice.path,
                  pageBuilder: (BuildContext context, GoRouterState state) =>
                      MaterialPage<void>(
                    key: state.pageKey,
                    child: ViewInvoicePage(url: state.queryParams['url']!),
                  ),
                ),
              ]),
        ],
        errorPageBuilder: (BuildContext context, GoRouterState state) =>
            MaterialPage<void>(
          key: state.pageKey,
          child: const HomePage(),
        ),
      );

While the redirect logic looks like this:

  String? _globalRedirect(GoRouterState state) {
    print(state.queryParams);

    // Incoming invoice handling.
    final String invoiceUrl = ref.read(incomingInvoiceProvider.notifier).url;
    if (invoiceUrl.isNotEmpty) {
      if (state.queryParams['url'] != invoiceUrl ||
          !state.subloc.contains(viewInvoice.path)) {
        return '/${viewInvoice.path}?url=$invoiceUrl';
      }
    }

    // Default
    return null;
  }

When navigating back from the ViewInvoicePage, the print statement would include the url query param, even though the path is now /.

csells commented 2 years ago

That's by design: go creates a whole stack of pages, passing each one the query params (who knows which route needs which parameters?). What happens when you call state.queryParams.clear() in the pageBuilder or redirect functions for the home route?

benPesso commented 2 years ago

Well, if there's a purposeful navigation occuring (by calling .goNamed, for example), the parameters should not persist. It should be assumed that the user will add whatever parameters are required for the new route.

The same is true if the use navigates back; when you navigate back in a browser, the browser does not retain your current set of params, but restores the previous set, if any.

That's [why] I suggested the addition of an onBackNavigation (or onPop) handler. So that we can clear the parameters before the new route is handled.

benPesso commented 2 years ago

BTW, I tried using state.queryParams.clear(), but it throws an exception: Exception: Unsupported operation: Cannot modify unmodifiable map.

The code for that is:

GoRoute(
  ...
  pageBuilder: (BuildContext context, GoRouterState state) {
    final String url = state.queryParams['url']!;
    state.queryParams.clear();

    return MaterialPage<void>(...);
  }
  ...
)

I also tried passing the state to the page (the child of MaterialPage) - which had a WillPopScope parent - to clear the state when the page is about to pop, but that resulted in the same exception being thrown.

csells commented 2 years ago

Hmm... I understand the desire to clear queryParams for sub-routes that don't need it. I'm not sure of a good way to handle this issue, however. Thoughts?

benPesso commented 2 years ago

Well, we can just make queryParams nullable. Or have namedLocation and its siblings ignore empty maps. Right now they return these maps in the queryParams value, even if empty.

benPesso commented 2 years ago

Just to add to this, if I navigate from / to a sub-route with some query param, when I "pop" the page by navigating back, the query params magically get attached to the / route, even though that was never explicitly set.

You can see this by attaching a handler to the GoRouter's redirect property and printing the state.queryParams. My test was something like this:

  1. Navigate to a generated destination.
    destination = state.namedLocation(
    '/view-link',
    queryParams: {'url': 'https://example.com'},
    );
  2. Print state.location and state.queryParams. (From redirect handler.)
    flutter: {Redirect triggered, /view-link?url=https%3A%2F%2Fexample.com, {url: https://example.com}}
  3. Swipe to go back.
  4. Trigger redirection check via refreshListenable. (No actual navigation occurs.)
  5. Print state.location and state.queryParams. (From redirect handler.)
    flutter: {Redirect triggered, /?url=https%3A%2F%2Fexample.com, {url: https://example.com}}
csells commented 2 years ago

Yes. All of this is by design. I agree it's not a great design...

benPesso commented 2 years ago

What was the design principal this was following? 🤔

It surely breaks that of the common web browser; where query parameters exist only in the URL for which they were explicitly added.

percula commented 2 years ago

Thank you for this great library! I'm having this issue too, just as benPesso wrote.

For example:

"example.com/contacts" -> "example.com/contacts/42?name=bob%20loblaw"

When I pop, I'm sent back to "example.com/contacts?name=bob". Obviously, I only need that query param on the detail page, not when I pop back to the list page.

However, navigating back using the browser back button goes back to "example.com/contacts". I feel like this is the ideal behavior for "pop" too. It would be great if it was possible for GoRouter to keep track of which QueryParams are added for each pushed route and clear them as the pages are popped. This way child routes can access parent query params, but parent routes aren't bothered with child's query params.

csells commented 2 years ago

I think there should be some facility in go_router to remove unwanted query parameters from URLs in these cases. I haven't yet figured out the best API to enable that feature, however.

raulmabe commented 2 years ago

Hi! I also have a use case where I need to clear queryParams.

I have a page that if a specific query param exists, it will open a submenu. At the same time the page keeps refreshing with the queryParam already used because of my refreshListenable. It will open the submenu every time it refreshes, even on new routes I may stack after. This makes dynamic linking harder.

I would need to clear the query param after I have extracted the value.

What do you think about making the state.queryParams modifiable? What cons may it have?

csells commented 2 years ago

I think making state.queryParams clearable is the right way to solve this problem.

benPesso commented 2 years ago

I think it should be automatically cleared on pop() AND be clearable. Since pop() doesn't maintain the state in Flutter, it doesn't make sense for GoRouter to behave differently.