Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: Ionic React useIonRouter returns unstable reference causing maximum update depth error in useEffect #24987

Closed
5 of 6 tasks
babycourageous opened this issue Mar 23, 2022 · 7 comments · Fixed by #25000
Closed
5 of 6 tasks
Labels
package: react @ionic/react package type: bug a confirmed bug report

Comments

@babycourageous
Copy link
Contributor

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x

Current Behavior

When using the router object returned from useIonRouter inside a useEffect, the router must be placed in the dependency array. The useIonRouter hook returns the object un-memoized so the dependency in useEffect causes an infinite loop.

Expected Behavior

The object returned by useIonRouter should be stable to be able to use in dependency arrays.

Steps to Reproduce

Run the repro - on the home page click the redirect button. It uses an effect to push to a new route. The router object is required in the dependency array and it causes the maximum update exceeded loop.

Code Reproduction URL

https://github.com/babycourageous/ionic-stable-useIonRouter

Ionic Info

Ionic:

Ionic CLI : 6.18.1 (/Users/renedellefont/.fnm/node-versions/v16.13.1/installation/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/react 6.0.13

Capacitor:

Capacitor CLI : 3.4.3
@capacitor/android : not installed
@capacitor/core : 3.4.3
@capacitor/ios : not installed

Utility:

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

System:

NodeJS : v16.13.1 (/Users/renedellefont/.fnm/node-versions/v16.13.1/installation/bin/node)
npm : 8.1.2
OS : macOS Big Sur

Additional Information

I locally replaced the useIonRouter supplied by Ionic with the following and it fixed the issue. The object returned is wrapped by useMemo so that it's a stable reference for any dependency arrays it is included in.

export function useIonRouter(): UseIonRouterResult {
  const context = useContext(IonRouterContext)
  return React.useMemo(
    () => ({
      back: context.back,
      push: context.push,
      goBack: context.back,
      canGoBack: context.canGoBack,
      routeInfo: context.routeInfo,
    }),
    [context.back, context.canGoBack, context.push, context.routeInfo]
  )
}

The github repro is Ionic 6 but I can confirm the same issue exists in Ionic 5.

Thanks!

@ionitron-bot ionitron-bot bot added the triage label Mar 23, 2022
@babycourageous
Copy link
Contributor Author

Oh - sorry meant to say happy to submit the PR. Just thought I would need to file the issue first. Thanks!

@liamdebeasi liamdebeasi added package: react @ionic/react package type: bug a confirmed bug report labels Mar 24, 2022
@ionitron-bot ionitron-bot bot removed the triage label Mar 24, 2022
@liamdebeasi
Copy link
Contributor

Thanks, I can reproduce this behavior. We are happy to review a PR.

@babycourageous
Copy link
Contributor Author

Thanks for confirming @liamdebeasi. I'll look over your CONTRIB docs and get something in the queue for ya!

@babycourageous
Copy link
Contributor Author

@liamdebeasi sorry to ping you here - just having a small issue following the PR guidelines.
I'm getting a missing file/folder error for the core directory when running build directly in packages/react

Error: ENOENT: no such file or directory, stat '/Users/babycourageous/active_work/code/ionic-framework/core/css'

Do i need to run the core folder build before doing a build inside of packages/react? Thanks!

@liamdebeasi
Copy link
Contributor

@ionic/react depends on @ionic/core, so you need to build core before you can build react. You can run npm run sync in the react directory prior to building react to copy over the built core changes.

babycourageous added a commit to babycourageous/ionic-framework that referenced this issue Mar 25, 2022
Wrap router object that is returned in a `useMemo` to provide stable reference for any dependency arrays it may be supplied to.

This would address and close ionic-team#24987
@liamdebeasi liamdebeasi added type: bug a confirmed bug report and removed type: bug a confirmed bug report labels Nov 7, 2022
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #25000, and a fix will be available in an upcoming release of Ionic Framework.

@ionitron-bot
Copy link

ionitron-bot bot commented Dec 18, 2022

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.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Dec 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: react @ionic/react package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants