next/router - push function changes between renders when called

[dan-cooke]

Bug report

Depending on the push function of useRouter causes an infinite loop when the push function is called from inside a useEffect

To Reproduce

1. Set up a catch-all optional dynamic route [[...slug]].js
2. Render the following component from this page
   Example component

const BugExample = () => {
    useEffect(() => {
        push('/[[...slug]]', '/123/test', { shallow: true })
        console.log('called');
     },[push])

    return null
}

You will see an infinite loop of called in the console

Expected behavior

Push should be a non-changing reference, so that the react diff algoirithm knows not to call side effects again uneccesarily

System information

• OS: macOs
• Browser: Chrome
• Version of Next.js: 9.5
• Version of Node.js: 13.2.0

_NEXT-1375

[dan-cooke]

Hi @jamesmosier - I would be of the opinion that the various router methods should be using useCallback

The React hooks lint rule will complain if you do not include all dependencies in the array - we would prefer to not have to disable this lint rule.

[alexhofmandahl]

I ran into a similar issue in my project. I created a custom hook to handle query parameters using next's router. It relied on a useEffect that had the router in the dependency array. It would be really helpful if the router's methods could use the useCallback hook so my custom hook could properly work.

[ciampo]

Same here — it feels like the push function could definitely be memoized to avoid this sort of problems

[dontsave]

just want to resurface this issue. it would be great for widely consumed callbacks like this to be memoized

[rpedroni]

Be careful with this - passed the last 4 hours investigating why my cache stopped working and it was due to the fact that push was inside a useMemo dependency list. This can lead to some wild unexpected problems down the component tree.

[stomvi]

Router dependency warnings are the last things ESLint complains in my codebases. I agree with the suggestions that push should be using callback.

[niksajanjic]

I don't know the internal workings of useRouter methods, but I would propose that this change applies to all methods router is exporting, not just router.push. If we need to use any of the provided methods in the useEffect call, we're currently forced to pass that method to the ref to avoid eslint warning of missed dependency inside useEffect.

[macrozone]

just run into the same problem with useRouter().replace. I had the impression that this is a memoized function.

Problem is with nextjs 11, the default linting rules will force you to include replace to the deps array. Which might result in endless loops.

Its good that nextjs 11 default to these rules, but it should therefore follow conventions to avoid pitfalls like this.

Edit: I had to disable // eslint-disable-next-line react-hooks/exhaustive-deps multiple useEffect calls. I think at this point, this is clearly a bug and not a matter of documentation.

other solution is that nextjs declares exceptions in esint rule for useEffect for the various router functions.

[steida]

As I see it, Next.js Router has to be split to router commends and route values.

[steve-taylor]

Hi @jamesmosier - I would be of the opinion that the various router methods should be using useCallback

The React hooks lint rule will complain if you do not include all dependencies in the array - we would prefer to not have to disable this lint rule.

Be careful. useCallback (and useMemo) are performance optimizations, not semantic guarantees. Memoizing router.push with useCallback will break if/when a future version of React decides to evict memoized callbacks. See React docs.

It seems like a good workaround would be to exclude push from the dependency array.

@jamesmosier this is unsafe in asynchronous scenarios, e.g.

const router = useRouter()

// Safe
useEffect(() => {
    if (!loggedIn) {
        router.push('/login')
    }
}, [loggedIn])

// Not safe (router.push could have changed)
useEffect(() => {
    checkLoggedIn().then(loggedIn => {
        if (!loggedIn) {
            router.push('/login')
        }
    })
}, [])

I quickly came up with this, which seems to have worked:

import { useRouter } from 'next/router'
import type { NextRouter } from 'next/router'
import { useRef, useState } from 'react'

export default function usePush(): NextRouter['push'] {
    const router = useRouter()
    const routerRef = useRef(router)

    routerRef.current = router

    const [{ push }] = useState<Pick<NextRouter, 'push'>>({
        push: path => routerRef.current.push(path),
    })

    return push
}

It returns a push function that's semantically memoized and therefore safe to use with useEffect, e.g.

const push = usePush()

useEffect(() => {
    checkLoggedIn().then(loggedIn => {
        if (!loggedIn) {
           push('/login')
        }
    })
}, [push])

[bnaoki]

@steve-taylor would you mind explaining why this is not safe i.e. what's the effect of calling router.push on an old router instance?

const router = useRouter()
 
// Not safe (router.push could have changed)
useEffect(() => {
    checkLoggedIn().then(loggedIn => {
        if (!loggedIn) {
            router.push('/login')
        }
    })
}, [])

[andrei-hameza]

I can suggest another workaround:

import Router from 'next/router'

// in component
useEffect(() => {
    if (!loggedIn) {
        Router.push('/login')
    }
}, [loggedIn])

Router can be used in useEffect only or outside of the React lifecycles. I understand this is not the best solution but I prefer to use it over eslint-disable-next-line react-hooks/exhaustive-deps.

Some details:
#18522 (comment)

[deivoff]

Hello everyone, are there any plans to fix this behavior?

[dlwjiang]

It's been over two years. This seems like an issue that will be encountered by anyone trying to sync state with url query params. so I'm surprised there isn't more activity here... Makes me think I'm doing url state syncing incorrectly or something...

Currently using the fix mentioned above:

import Router from 'next/router'

[exneval]

const router = useRouter();
 
useEffect(() => {
  router.push("/");
}, [router])

router as hooks dependency (since eslint rules mad about not including it) is not stable to use even in v13.x.x.

@balazsorban44

[mkcode]

Just lost a few hours on this bug. Why has this not been fixed yet? Using next@13.1.1

[timneutkens]

This is fixed in the new router that is part of the app directory. We've changed the router in pages to be a stable reference a few months ago but it broke many existing applications so instead we've focused on making the new router have the right behavior as it'll become the recommended way to build Next.js applications when it's stable. The new router has other benefits like being built on top of React transitions and is concurrent rendering compatible.

[mkcode]

Thanks for the quick response @timneutkens. I look forward to migrating to the app directory when it's stable.

For the record, If the code is already available, I would think an opt-in via an explicit import would be the way to fix this without breaking existing apps, though I understand that this may not be worth the maintenance effort at this point.

import { useRouter } from 'next/router-new-push'

[timneutkens]

For the record, If the code is already available, I would think an opt-in via an explicit import would be the way to fix this without breaking existing apps, though I understand that this may not be worth the maintenance effort at this point.

Unfortunately it's not the useRouter that is the problem in pages. It's the router itself that changes between navigations as the first version was built in 2016 at which point it did not have to consider stable references across navigations, even more so import Router from 'next/router' still exists which mutates as well. Hence why App Router doesn't have router.query and such, instead it just exposes the minimal amount of apis.

However, since we provide a backwards compat layer for next/navigation, I was able to fix this issue by making sure useRouter from next/navigation is a stable reference across renders. I've opened a PR here: #52177.

Once that lands all you have to do is change to use import { useRouter } from 'next/navigation' 👍

[github-actions]

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.