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
50.98k stars 13.52k forks source link

bug: `match.params` is always an empty object when using `Redirect` in `IonRouterOutlet` #23743

Open Nevvulo opened 3 years ago

Nevvulo commented 3 years ago

Prequisites

Ionic Framework Version

Current Behavior

When rendering a Redirect component in IonRouterOutlet, when navigating to other pages that utilise parameters in the URL (for example, /tab1/:id), the match.params object will always be empty. This means that parameters cannot be extracted using useParams or reading the props.match.params object directly.

Expected Behavior

After a Redirect component is rendered and it takes the user to the desired page, I would expect any subsequent page visits to be populated with parameters if they are in the URL.

Steps to Reproduce

To reproduce using the code reproduction repo listed:

1) Navigate to localhost:8100 (or the test app) in a new tab. This should load tab 2. 2) Click on "Tab 1". This should take you to /tab1/TESTING. 3) The page should read "Tab 1 with param: undefined" when it should actually say "Tab 1 with param: TESTING".

Code Reproduction URL

https://github.com/Nevvulo/ionic-router-redirect-issue

Ionic Info

Ionic:

Ionic CLI : 6.16.3 (/Users/bswar/.nvm/versions/node/v14.16.0/lib/node_modules/@ionic/cli) Ionic Framework : @ionic/react 5.6.13

Utility:

cordova-res : not installed globally native-run : not installed globally

System:

NodeJS : v14.16.0 (/Users/bswar/.nvm/versions/node/v14.16.0/bin/node) npm : 6.14.11 OS : macOS Big Sur

Additional Information

Related issues: https://github.com/ionic-team/ionic-framework/issues/22230

It's worth mentioning that the official documentation for v5 states that this is valid for a fallback route: https://ionicframework.com/docs/react/navigation#fallback-route

Here is the diff containing just the changes that introduce the problem: https://github.com/Nevvulo/ionic-router-redirect-issue/commit/dfb52bacad714864a8faf1ae435d456eb0a5b956

gugahoi commented 3 years ago

This works in v4 by the way so it feels like a regression.

gugahoi commented 3 years ago

Bump? This is very unfortunate if this is how it meant to work. Some instructions from the team would be very much welcome.

brandonwestcott commented 2 years ago

Any update on this?

Also note using history.replace inside of a fallback route results in the same behavior

brandonwestcott commented 2 years ago

Still broken in 6.0.0-rc.2 as well

brandonwestcott commented 2 years ago

Another debug note, history.push also results in the failed case above

Nevvulo commented 2 years ago

Is it possible to get any update on this? This bug is still relevant in v5 (and v6 it seems) and is affecting our ability to update our codebase from Ionic 4.

thomasklemm commented 2 years ago

Ran into this with Ionic React 6.0.2, didn't find a workaround yet.

Edit: It looks like matchPath could be a workaround as described here: https://github.com/remix-run/react-router/issues/5870#issuecomment-394194338

thomasklemm commented 2 years ago

One solution that seems to work for now is using a <Switch> component inside of <IonRouterOutlet>, though the Ionic React Routing docs say it shouldn't make a difference. Idea taken from another Ionic user (@Chadh93 in Discord) who reported his findings in Discord: https://discord.com/channels/520266681499779082/520358616646025223/924416095539069008

thomasklemm commented 2 years ago

Created a minimal reproduction of this bug including a screen recording, where the match.params are empty if a redirect via a fallback route has happened at the beginning of the session.

Wrapping the routes in a <Switch> component from react-router inside of <IonRouterOutlet> fixed the situation for me, the fallback route is now working, params get parsed properly (Found this solution via this message in Discord)

moekify commented 2 years ago

Encountered this exact same issue. It's really annoying to work with unfortunately :(

thomasklemm commented 2 years ago

Feedback from my side: Adding the <Switch> component in broke some other behaviour (can't exactly remember what). Went removing it again and having no fallback routes in the app at the moment (which might be fine for a mobile app, but definitely not fine for a web SPA). Will have to revisit this

wmadden commented 2 years ago

@thomasklemm using <Switch> instead of <IonRouterOutlet> means there will be no transitions between pages.

It's shocking the Ionic team hasn't even responded to this bug in the router since it was raised in 2018.

Bump

DySQRD commented 2 years ago

Just ran into this issue in 6.19.1. useIonViewWillLeave and useIonViewDidLeave also stop working. Another workaround for web pages, if you don't mind a refresh : <Redirect to={() => {window.location.replace(REF); return <AnyComponent />;} /> // where REF is a valid url

Please look into this, Ionic Team !

e-monson commented 2 years ago

Is this framework dead or what?

llostris commented 1 year ago

Any updates on this? 👀

We just ran into this on 7.2.2 in these two scenarios:

  1. Redirect from /login to /home page when user is logged in – after the redirect our params on Home page would come up as undefined.
  2. When handling a default 404 route. If user then goes to Home page by clicking the Home link, the params on Home page come up as undefined.

In both cases a refresh would result in params being loaded properly, so it's something specific to redirect and/or the use of the default route.

We can confirm that the <Switch></Switch, hack from this message does solve the problem, but as said above – this impacts transitions between pages.

legendar commented 1 year ago

This happens not only for <Redirect> but for any <Route> without the path param. The issue is somewhere in Ionic StackManager. Looks like it incorrectly calculates the previous view in some cases.

E.g. with the following routes:

<Route path="/home/:someParam"><Home /></Route>
<Route><NotFound /></Route>

If we start from any "not found" URL and then navigate to /home the Ionic StackManager will calculate enteringViewItem the same as was rendered for NotFound component and will replace the entering Route element but will leave all other props as is including computedMatch values which are empty. This is caused by the following line: https://github.com/ionic-team/ionic-framework/blob/6d4c52aa5bbafd390056eb57a9151c55f9788c17/packages/react-router/src/ReactRouter/ReactRouterViewStack.tsx#L127 but this behaviour can't be disabled otherwise routes without path params will not work.

I think this one also related https://github.com/ionic-team/ionic-framework/issues/26524

Switch from ReactRouter always recalculated computedMatch this is why it works as a workaround. But in this case, the component tree will affected and some features from the Ionic Router will break (e.g. transitions and life-circle hooks). Route component from React Router can recalculate match values if computedMatch is not provided. So the better workaround will be to remove computedMatch calculations:

import {Route as ReactRouterRoute} from 'react-router'

function Route({computedMatch, ...props}) {
  return <ReactRouterRoute {...props} />
}
legendar commented 1 year ago

After further debugging, we noticed that the workaround of removing computedMatch can lead to other unexpected issues. The problem is that the page component which is rendered via Router will be recreated several times as props will be changed when Route will recalculate computedProps. As a result, we encountered transition flickering issues and extra requests to API.

After deeper investigation, we found another more complex workaround and looks like it covers all the issues. The main idea is to fix findViewItemByRouteInfo method from ReactRouterViewStack so it will never return invalid view item. This method is provided via context so we can easily patch it.

import {
  forwardRef,
  useContext,
  useMemo,
} from "react";

import {
  IonRouterOutlet,
  RouteManagerContext,
} from "@ionic/react";
import isEqual from "lodash/isEqual";
import omit from "lodash/omit";

function IonRouterOutletPatched({children, ...otherProps}, ref) {
  const routeManagerContextValue = useContext(RouteManagerContext);
  const findViewItemByRouteInfoOriginal =
    routeManagerContextValue.findViewItemByRouteInfoOriginal ||
    routeManagerContextValue.findViewItemByRouteInfo;
  const createViewItemOriginal =
    routeManagerContextValue.createViewItemOriginal ||
    routeManagerContextValue.createViewItem;
  const newValue = useMemo(
    () => ({
      ...routeManagerContextValue,
      findViewItemByRouteInfo(routeInfo, outletId, updateMatch) {
        const viewItem = findViewItemByRouteInfoOriginal(
          routeInfo,
          outletId,
          updateMatch
        );
        // `matchRoute` is a function from Ionic StackManager
        // https://github.com/ionic-team/ionic-framework/blob/main/packages/react-router/src/ReactRouter/StackManager.tsx#L433
        // unfortunatly it is not exported so we just copy it to our utils file
        const routeElement = matchRoute(
          children,
          routeInfo
        );
        if (
          viewItem &&
          routeElement &&
          isEqual(
            viewItem.initialRouteProps,
            omit(routeElement.props, "children")
          )
        ) {
          return viewItem;
        }
        // otherwise undefined will be returned and Ionic will create new viewItem
      },
      createViewItem(outletId, routeElement, routeInfo, page) {
        const viewItem = createViewItemOriginal(
          outletId,
          routeElement,
          routeInfo,
          page
        );
        // we want to know the route props asociated to current viewItem
        // so we can find correct viewItem later
        viewItem.initialRouteProps = omit(routeElement.props, "children");
        return viewItem;
      },
      // save original context methods to ensure that we never lead to recursion if you use nested routes
      findViewItemByRouteInfoOriginal,
      createViewItemOriginal,
    }),
    [routeManagerContextValue]
  );

  return (
    <RouteManagerContext.Provider value={newValue}>
      <IonRouterOutlet ref={ref} {...otherProps}>
        {children}
      </IonRouterOutlet>
    </RouteManagerContext.Provider>
  );
}

// forwarding ref to ensure that IonRouterOutlet will work as expected in any scenario
const IonRouterOutletPatchedWithForwardedRef = forwardRef(
  IonRouterOutletPatched
);
// IonTabs children should be IonRouterOutlet or element with isRouterOutlet=true
// https://github.com/ionic-team/ionic-framework/blob/71a7af0f52fe62937b1dea1ca2739e78801a2a6d/packages/react/src/components/navigation/IonTabs.tsx#L106
IonRouterOutletPatchedWithForwardedRef.isRouterOutlet = true;
export default IonRouterOutletPatchedWithForwardedRef;

All you need is just to use IonRouterOutletPatched instead of IonRouterOutlet. Please note that it does not support any other children types expect of Route element, so you can't use Fragment inside.

Hope the Ionic team can add a quick patch based on this solution 👍

Elardzhi commented 12 months ago

@legendar

I tried that solution but it causes another issue:

Maximum update depth exceeded. 
This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. 
React limits the number of nested updates to prevent infinite loops.
legendar commented 12 months ago

@Elardzhi

We using this workaround in our project and all things working fine. I'm not sure why it is not working for you. Please make sure that you are using only Route nodes as direct children for IonRouterOutletPathced. Also, you need to copy matchRoute function from Ionic StackManager code and add the matchPath import from react-route:

import {matchPath} from "react-router";

Please also note that we are using "@ionic/react": "^7.3.3", and I have not tested this on other versions. Maybe I can help you if you will share the code.

adamschoenemann commented 11 months ago

Bump for this one! It's a rather serious issue that's been going on for years now, so I'm really hoping it could be prioritized :crossed_fingers:

NickAlvesX commented 11 months ago

If anyone needs a version from @legendar's solution working with Typescript:

import React, { forwardRef, useContext, useMemo } from 'react';

import isEqual from 'lodash/isEqual';
import omit from 'lodash/omit';

import {
  IonRouterOutlet,
  RouteInfo,
  RouteManagerContext,
  RouteManagerContextState,
  ViewItem,
} from '@ionic/react';

import { matchRoute } from './utils';

type PatchedViewItem = ViewItem & { initialRouteProps?: any };

const IonRouterOutletPatched = (
  { children, ...otherProps }: React.PropsWithChildren<any>,
  ref: React.Ref<any>,
) => {
  const routeManagerContextValue: RouteManagerContextState & {
    findViewItemByRouteInfoOriginal?: () => ViewItem;
    createViewItemOriginal?: () => ViewItem;
  } = useContext(RouteManagerContext);

  const findViewItemByRouteInfoOriginal =
    routeManagerContextValue.findViewItemByRouteInfoOriginal ||
    routeManagerContextValue.findViewItemByRouteInfo;
  const createViewItemOriginal =
    routeManagerContextValue.createViewItemOriginal || routeManagerContextValue.createViewItem;
  const newValue = useMemo(
    () => ({
      ...routeManagerContextValue,
      findViewItemByRouteInfo(routeInfo: RouteInfo, outletId?: string, updateMatch?: boolean) {
        const viewItem: PatchedViewItem | undefined = findViewItemByRouteInfoOriginal(
          routeInfo,
          outletId,
          updateMatch,
        );
        // `matchRoute` is a function from Ionic StackManager
        // https://github.com/ionic-team/ionic-framework/blob/main/packages/react-router/src/ReactRouter/StackManager.tsx#L433
        // unfortunately it is not exported, so we just copy it to our utils file
        const routeElement = matchRoute(children, routeInfo);
        if (
          viewItem &&
          routeElement &&
          isEqual(viewItem.initialRouteProps, omit(routeElement.props, 'children'))
        ) {
          return viewItem;
        }
        // otherwise undefined will be returned and Ionic will create new viewItem
      },
      createViewItem(
        outletId: string,
        routeElement: React.ReactElement,
        routeInfo: RouteInfo,
        page?: HTMLElement,
      ): PatchedViewItem {
        const viewItem: PatchedViewItem = createViewItemOriginal(
          outletId,
          routeElement,
          routeInfo,
          page,
        );
        // we want to know the route props associated to current viewItem
        // so we can find correct viewItem later
        viewItem.initialRouteProps = omit(routeElement.props, 'children');
        return viewItem;
      },
      // save original context methods to ensure that we never lead to recursion if you use nested routes
      findViewItemByRouteInfoOriginal,
      createViewItemOriginal,
    }),
    [routeManagerContextValue],
  );

  return (
    <RouteManagerContext.Provider value={newValue}>
      <IonRouterOutlet ref={ref} {...otherProps}>
        {children}
      </IonRouterOutlet>
    </RouteManagerContext.Provider>
  );
};

// forwarding ref to ensure that IonRouterOutlet will work as expected in any scenario
const IonRouterOutletPatchedWithForwardedRef: React.ForwardRefExoticComponent<any> & {
  isRouterOutlet?: boolean;
} = forwardRef(IonRouterOutletPatched);
// IonTabs children should be IonRouterOutlet or element with isRouterOutlet=true
// https://github.com/ionic-team/ionic-framework/blob/71a7af0f52fe62937b1dea1ca2739e78801a2a6d/packages/react/src/components/navigation/IonTabs.tsx#L106
IonRouterOutletPatchedWithForwardedRef.isRouterOutlet = true;
export default IonRouterOutletPatchedWithForwardedRef;

and the matchRoute util:

import React from 'react';

import { matchPath } from 'react-router';

import { RouteInfo } from '@ionic/react';

export const matchRoute = (
  node: React.ReactNode,
  routeInfo: RouteInfo,
): (React.ReactNode & { props?: any }) | null | undefined => {
  let matchedNode: React.ReactNode;
  React.Children.forEach(node as React.ReactElement, (child: React.ReactElement) => {
    const matchProps = {
      exact: child.props.exact,
      path: child.props.path || child.props.from,
      component: child.props.component,
    };
    const match = matchPath(routeInfo.pathname, matchProps);
    if (match) {
      matchedNode = child;
    }
  });

  if (matchedNode) {
    return matchedNode;
  }
  // If we haven't found a node
  // try to find one that doesn't have a path or from prop, that will be our not found route
  React.Children.forEach(node as React.ReactElement, (child: React.ReactElement) => {
    if (!(child.props.path || child.props.from)) {
      matchedNode = child;
    }
  });

  return matchedNode;
};
NickAlvesX commented 10 months ago

I noticed that this solution is still causing problems for me sometimes, like:

Uncaught (in promise) Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
    at checkForNestedUpdates (react-dom.development.js:23583:1)
    at scheduleUpdateOnFiber (react-dom.development.js:22019:1)
    at Object.enqueueForceUpdate (react-dom.development.js:12338:1)
    at Component.forceUpdate (react.development.js:335:1)
    at StackManager.handlePageTransition (index.js:345:1)
    at StackManager.componentDidUpdate (index.js:242:1)
    at commitLayoutEffectOnFiber (react-dom.development.js:20200:1)
    at commitLayoutMountEffects_complete (react-dom.development.js:21341:1)
    at commitLayoutEffects_begin (react-dom.development.js:21322:1)
    at commitLayoutEffects (react-dom.development.js:21276:1)

This seems to be because my this.routerOutletElement inside the StackManager became null and it keeps reloading.

NickAlvesX commented 10 months ago

I found something. The reason why sometimes I get an infinity loop is because of this:

if (
  viewItem &&
  routeElement &&
  isEqual(
    viewItem.initialRouteProps,
    omit(routeElement.props, "children")
  )
) {
  return viewItem;
}
// otherwise undefined will be returned and Ionic will create new viewItem

More precisely this: isEqual(viewItem.initialRouteProps, omit(routeElement.props, "children"))

This not always evaluates to true when it should.

In my specific case, this infinity loop is only happening for the routes that I have inside a Suspense that has a fallback prop, and react-router sends this prop to my viewItem (ref about this here)

This fallback prop is huge and isEqual(viewItem.initialRouteProps, omit(routeElement.props, "children")) sometimes evaluates 2 fallback props to equal, sometimes it doesn't, and I don't know exactly when this happens.

When this isEqual fails, I get the infinity loop. You can easily reproduce it by replacing it with false.

So my questions to @legendar specifically are:

NickAlvesX commented 10 months ago

I have fixed the infinite loop by omitting the fallback prop by doing omit(routeElement.props, ['children', 'fallback']).

legendar commented 10 months ago

@NickAlvesX This check is the main idea of the workaround =) We compare route props that were used when creating the current viewItem (initialRouteProps) with current route props (routeElement.props). If they are not equal then ionic calculated the wrong viewItem and we return undefined. children was omitted to prevent an infinite loop as they are always not equal. I think render and component props should be omitted as well. We don't use any other render methods except children in our project so this is why this code works for us. But yeah all other non-standard methods (like fallback in your case) should also be omitted. Alternatively, instead of omitting those props, we can define which props we need to compare as we only need to check route configuration props like path, isExact, strict, sensitive.

if (
  viewItem &&
  routeElement &&
  isEqual(
    viewItem.initialRouteProps,
    pick(routeElement.props, ["path", "isExact", "strict", "sensitive"])
  )
) {
  return viewItem;
}
nadavhalfon commented 3 months ago

Any update on this?

nadavhalfon commented 1 month ago

How come this fundamental issue doesn't get resolved for 3 years? If ionic suppose to support react so be it, but it seems like react abounded

brandonwestcott commented 2 weeks ago

Just started a fresh v8 project, and I was quite surprised to see this was still and issue.

@brandyscarney @thetaPC can we get some guidance on how we should be handling fallback routes (ex 404) in our ionic apps?