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)