ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
51.13k stars 13.5k forks source link

feat: Make react router work with nested routes #22992

Closed mauriceackel closed 3 years ago

mauriceackel commented 3 years ago

Feature Request

Ionic version: [x] 5.5.5

Describe the Feature Request Although explained in the docs, the ionic router for react is majorly broken when using it with nested routes in the most recent ionic version. This is especially the case if it is combined with ionic tabs. For my case, this renders the ionic routing completely useless due to unforeseeable bugs that occur while usage (i.e. pages keep hidden, broken navigation animations, etc.).

Describe Preferred Solution IonRouterOutlets should be nestle inside each other and one ion router outlet should only control the state of the nested pages. Especially, the nested router outlets need to be aware of them being nested as right now, the z-index changes fail for nested routes. Furthermore, animations should be completely disabled for pages directly nested in a tab view. Right now, there are scenarios in which programmatically changing the route using history.push(), animations could happen even in a tab view. Most of the issue are primarily visible when viewing the page in iOS mode.

Describe Alternatives I know that is is an extremely broad request but trust me, I tried to narrow it down to actual bug reports but there is just too much broken and too many cases to be considered. I would love to see a working router that is actually usable for projects that are not just demos. I worked myself through a lot of bug reports regarding this topic on GitHub in the last weeks and until now it always seems that there are only minor fixes but nothing that resolves the underlying issues :(

liamdebeasi commented 3 years ago

Thanks for the issue. Ionic React does support nested router outlets. I understand you are having a hard time breaking this down to bug reports, but without being able to reproduce these issues on my end it is hard to fix any issues here.

Could you maybe start by giving me your use case where you need nested IonRouterOutlet components and we can work from there?

liamdebeasi commented 3 years ago

Also if you have an example app of things not working, that would be helpful as well.

mauriceackel commented 3 years ago

Hi @liamdebeasi sure, I can understand that! Sorry for being so broad but at this point I am just completely lost. I am going to push some code to an example repo and link it here soon.

The use case is to have protected parts of an app. Let's say /protected/a and /protected/b. In the documentation, you do so by having routes that conditionally render components based on the auth state of a user. I want to do the same in my application. I have to use nested routes for this because otherwise I would need to add the route guard to each and every protected route, which is unfeasible and rather ugly :D

What I tried is to simply render the protected routes conditionally without having nested router outlets at all. However, if routes in a router outlet are wrapped inside a react fragment <></> page transition animations don't work. Hence, this approach does not work for me...

As said, I am going to create a demo repo for you now and post it here with some more instructions on how to reproduce the issues.

mauriceackel commented 3 years ago

@liamdebeasi Here we go: https://github.com/mauriceackel/react-routing-issue-example

I created the app using the command from the docs (i.e. ionic start photo-gallery tabs --type=react --capacitor).

I tried to explain what to do and what issues to expect on the individual pages as best as possible. Let me know if this works for you.

There are actually more issues in my real app where for example the ion-page-hidden class does not get correctly removed when navigating between tabs using history.replace() if history is included like this const history = useHistory() but for some reason works if the history is used from the RouteComponentProps. I'm trying to add this to the example.

mauriceackel commented 3 years ago

@liamdebeasi Just FYI, I added another case (Tab 4) That shows a broken router animation between a nested page in a tab and a page that is also in the route outlet of the IonTabs component. Here, I just wanted to add that the animation works fine if tab4 is not nested but breaks if it is nested (you can try this but simply changing the default export in Tab4.tsx)

liamdebeasi commented 3 years ago

Thank you! This was very helpful. I took a look and here are some thoughts:

  1. I don't think you even need a nested router outlet here. Nested routes are mostly useful when you need to render content in outlet A while also rendering sub-content inside of a nested outlet B. The most common use case you will run into is tabs (rendering a tab bar in outlet A and tab content in outlet B). I don't really see anything like that happening with your subpages, so I think it makes more sense to write these as sibling routes in App.tsx.
<IonApp>
  <IonReactRouter>
    <IonTabs>
      <IonRouterOutlet>
        <Route path="/tab2/" exact={true}>
          <Tab2 />
        </Route>
        <Route path="/tab2/sub/" exact={true}>
          <Tab2Sub />
        </Route>
      </IonRouterOutlet>

      ...

    </IonTabs>
  </IonReactRouter>
</IonApp>

Be sure to include exact={true} otherwise React Router may match Tab2Sub when on the /tab2/ path.

  1. Clicking the "To tab 1" button on Tab 3 and having to animation does seem to be inconsistent, but you can workaround this for now by adding routerDirection="none" to the IonButton. This seems to be specific to Ionic React as I cannot reproduce this in Ionic Vue or Ionic Angular. Need to look into this a bit more.

  2. I don't see anything particularly wrong in your app that would cause the z-index values to change that way, but I don't think it should negatively impact anything since the leaving views are hidden using display: none when the animation finishes.

Overall I think the main issue here is that you are trying to make nested routes inside of tabs when they should really be sibling routes, so addressing my first point should fix most of the issues you are seeing. Can you give that a try and let me know how it goes?

mauriceackel commented 3 years ago

Hey @liamdebeasi, thanks for looking into this! As I said, although sibling routes would perhaps work fine, what would you do if you want to make /tab2/ and all its sub-pages protected? If I would do it as sibling routes, I would need to add a guard on each and every route instead of simply protecting the /tab2/ route and having all nested routes protected with it.

mauriceackel commented 3 years ago

I tried to achieve said route protection by putting the routes I want to protect in a conditionally rendered fragment but unfortunately this completely breaks the route animations between the pages :(

liamdebeasi commented 3 years ago

I think the easiest thing would be to create a PrivateRoute component that you can use alongside the regular Route component. I've seen non-Ionic devs using React do this as well.

Here is an example: https://stackoverflow.com/questions/66193574/route-guard-while-using-ionic-react

Basically you would create a PrivateRoute component and then use that in place of Route on routes that you want to protect.

liamdebeasi commented 3 years ago

If you are running into issues with animations I would recommend trying to render out a <Route> rather than the path's component directly. So something like this would go in your PrivateRoute component:

if (loggedIn) {
  return <Route {...props} />;
} else {
  return <Redirect to={redirect} />;
}
mauriceackel commented 3 years ago

Thanks a lot for the input, I have something similar in place right now but I guess I can make it suitable to work on all routes by extracting the signin-check logic to a context like the example you referenced.

Nevertheless, I think there might be a bit of confusion not oust from me about the whole nested route topic, as I think many others try to use nested routes in a similar way to me, perhaps also for better code structuring (I guess in angular it is done in a similar way, no?). Perhaps the docs could be a bit clearer around this whole topic, right now it is rather hard to comprehend, at least that is my experience.

mauriceackel commented 3 years ago

Hi @liamdebeasi! So I implemented the changes you suggested regarding the private route what I do is to return a different route depending on whether the suer is logged in or not. Unfortunately this raises another issue:

I have (among others) 2 tabs (/account/ and /checkin/) that are protected by this PrivateRoute component. Now if I am not logged in and change between those two tabs, the ion router outlet of the IonTab component produces 2 pages and correctly handles their z-index and visibility. However, now that those two routes are rendered, if I log in, both private routes will render out their respective "real" routes. The Ion router outlet does not handle this correctly (or perhaps is simply not intended to do so) as it now looses track of which page has to be visible and which not.

From what I understand about the router, this is kind of by-design, I mean how could the router outlet know which page has to be visible and which not if suddenly two new pages get rendered that it has never seen before.

Part of my private route component:

  if (!authenticated) {
    return <Route {...props} component={SignIn} />;
  }

  if (!authorized) {
    return <Route {...props} component={Unauthorized} />;
  }

  return <Route {...props} />;

You can see the behavior in the following clip (I suggest watching it frame by frame or simply look at the initial state and the final state).

https://user-images.githubusercontent.com/20804369/109712661-9a618c00-7ba0-11eb-8d3e-66a0656873f5.mov

mauriceackel commented 3 years ago

Some update after more testing.. You know when eslint tells you that you should avoid prop spreading? Maybe there is a deeper reason to this :D I removed prop spreading from the routes and instead set the properties manually from the destructured properties of the PrivateRoute:

<Route
  path={path}
  exact={exact}
  strict={strict}
  location={location}
  sensitive={sensitive}
  component={SignIn}
/>
...

And now instead of having two IonPages rendered and managed by the ion router outlet (like in my video). There is only one being rendered even when tabs are changed. This way, the issue does not longer arise as there can't be two IonPages that suddenly have the same CSS properties.

Anyhow, I still consider the behavior from my previous post an issue that might require some deeper testing and understanding to prevent such concurrent cases also in other scenarios. What do you think @liamdebeasi?

mauriceackel commented 3 years ago

Soooo, the journey continues :D

The very same thing is praised in any last comment as a solution is actually not a solution at all and makes it worse. As I explained above, in the current setup the IonRouteOutlet inside the IonTabs component only keeps one of my tabs alive (i.e. in the DOM). While this fixes the issue explained above, unfortunately the same behavior appears for routes with children. This means that if I have two routes /account/ and /account/details and I navigate from account to details?, the account page gets completely removed from the DOM. This of course breaks the animations between the pages (see attached video).

https://user-images.githubusercontent.com/20804369/109836504-81aab200-7c44-11eb-8460-dfee1465f1a0.mov

From my understanding the ion router outlet should maintain the state of 'parent' pages when navigating to sub-pages, which does not happen here. This behavior appears bot times, with and without using /:tab(account)/ as explained in the docs.

My Code:

<IonTabs>
  <IonRouterOutlet>
    <PrivateRoute strict exact path="/:tab(account)/" component={Account} />
    <PrivateRoute
      strict
      exact
      path="/:tab(account)/details/"
      component={Details}
    />
  </IonRouterOutlet>
...
</IonTabs>

Inside PrivateRoute:

...
if (!authenticated) {
  return (
    <Route
      path={path}
      exact={exact}
      strict={strict}
      location={location}
      sensitive={sensitive}
      component={SignIn}
    />
  );
}

if (!authorized) {
  return (
    <Route
      path={path}
      exact={exact}
      strict={strict}
      location={location}
      sensitive={sensitive}
      component={Unauthorized}
    />
  );
}

return (
  <Route
    path={path}
    exact={exact}
    strict={strict}
    location={location}
    sensitive={sensitive}
    component={component}
  />
);
...
mauriceackel commented 3 years ago

Some testing later:

It seems as if the IonRouter outlet does not keep the page alive if the Route is not defined as a direct child of the IonRouter Outlet.

This is: If a with a conditional render function is placed in the IonRouterOutlet, the outlet keeps the page alive when switching tabs. If instead the component (which itself returns a component) is defined in the IonRouterOutlet, the outlet does not keep the page alive when switching tabs.

As a result, your proposed solution @liamdebeasi with the private route does not seem to work with IonTabs and React, do you consider this a bug?

liamdebeasi commented 3 years ago

Yeah that PrivateRoute issue is definitely a weird one. I have not dug deep into your other issues but in an attempt to make it easier to keep track of things can we close this in favor of the https://github.com/ionic-team/ionic-framework/issues/23013 and https://github.com/ionic-team/ionic-framework/issues/23012?

For https://github.com/ionic-team/ionic-framework/issues/23012 I think this is a known bug but I need to check with the team. For https://github.com/ionic-team/ionic-framework/issues/23013 I can reproduce this, but I am trying to figure out why it happens.

mauriceackel commented 3 years ago

Sure, lets close this one, it developed into more of a personal diary lately :P

ionitron-bot[bot] commented 3 years ago

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.