Should router be in the dependency array of useEffect?

[moroshko]

useRouter docs show examples where router is not part of the dependency array of useEffect, e.g.:

export default function Page() {
  const { user, loading } = useUser()
  const router = useRouter()

  useEffect(() => {
    if (!(user || loading)) {
      router.push('/login')
    }
  }, [user, loading])  // why `router` is not here?

  return <p>Redirecting...</p>
}

ESLint clearly complains about this:

React Hook useEffect has a missing dependency: 'router'. Either include it or remove the dependency array. react-hooks/exhaustive-deps

Is it recommended to // eslint-disable-next-line react-hooks/exhaustive-deps here?

Does useRouter() guarantee to return the same router instance every time? If so, I guess it shouldn't hurt adding router to the dependency array.

I'm wondering why the Next.js docs mentioned above explicitly exclude router from the dependency array if ESLint complains about this 🤔

[thibautsabot]

Hello!

useRouter is a React context that wraps everything.

So in most cases, two useRouter calls will be the same thing.

Very simple example:

import { useRouter } from "next/router";

const Home = () => {
  const router = useRouter();
  const router2 = useRouter();

  console.log(router === router2); // will be true
  return <div />;
};

export default Home;

That being said, what is the actual purpose of the eslint rule ?

The rule should be here as a warning to inform you that you may have made a mistake.

The dependencies array of useEffect indicates which variable change should trigger an effect or not.

In your case, do you want your effect to run whenever the user or the loading change ? Absolutely.
But do you want to redirect the user to /login when the router object changes ? Maybe not.

---

I believe the dependencies array should be different for every use-case.

Perhaps you want to redirect the user if the id part of your URL changes ? Then you could use router.query.id in your useEffect (like here).

Or you only want to do something once (on mount), like an API call? Your array will be empty. Even if the eslint rule doesn't like it:

useEffect(() => {
  // Do something
}, [])

This is a personal opinion, but eslint rules should help a developper do his job, it should not make his life harder.
I have this rule as a warning on my projects (and so is Next.js).

But to be honest, in your example, it doesn't really matter 😅 .

[spikebrehm]

I stumbled across this discussion when noticing a bug in my app due to expecting a useEffect() that depended on router to only fire once. Something like this in my _app.tsx:

  useEffect(() => {
    AutoUpdateController.initialize(router);
  }, [router]);

This useEffect() would run on every page navigation, which led to a bug. Why? Because router appears to not be referentially stable across navigations.

To debug referentially stable values between renders, you can test using a usePrevious() hook like:

// usePrevious.ts
import { useEffect, useRef } from 'react';

export default function usePrevious<T>(value: T) {
  const ref = useRef<T>();
  useEffect(() => {
    ref.current = value;
  }, [value]);
  return ref.current;
}

and in your component do something like:

  const router = useRouter();
  const prevRouter = usePrevious(router);
  if (prevRouter) {
    console.log(router === prevRouter);
  }

Which makes it clear that the value of useRouter() is not referentially stable, and likely why in all of the useEffect() examples in the docs, router is left out of the dependencies array.

IMO this is a pretty big footgun that should be called out. Assuming it's not feasible to actually make router referentially stable across navigations (although naively I would think that should be possible), the docs should at least be updated to mention this.

There's even a Potential ESLint errors section of the docs! Seems like a great place to mention this rules-of-hooks ESLint error that everyone will get. (cc: @leerob)

[SpencerKaiser]

So for someone who currently has this issue, what's the best way to resolve it? I don't want to include it in my useEffects dependency array, but I also don't want to disable my eslint rule for that line because it could cause me to accidentally forgot to include another dependency I actually need...

[grncdr]

@SpencerKaiser I didn't realise Valentins link was directly to his comment and not the issue description, it probably makes the above discussion a little confusing.

I've added clearer instructions on how to try and configure my fork of the eslint plugin to my first comment here: facebook/react#16873 (comment). Give it a try and leave a comment/thumbs up etc if everything works for you.

[SpencerKaiser]

@grncdr thanks for the reply! So I read through a good deal of that thread and here's my takeaway, let me know if I missed the mark:

• Yall are waiting for a more formal solution, but it seems like it's been quite a while without real traction from the maintainers
• The only way to "safely" address it now is to add the package you created and then mark that hook as static

I'm still semi-new to eslint in terms of the customization side of things, so I'm a little skittish about adding a new package when things are stable the way they are 😕

[grncdr]

@SpencerKaiser yep, you've got it.

I'm not a fan of opening unsolicited PRs on high-traffic repos. I'm super happy with the way it works for me (and I think a few other users are too) but for all I know the React team might think it's a terrible idea. 🤷

Regarding trying out the other package, that's a totally reasonable position to have. Depends a lot on your team and environment. If I had to convince 20 other developers I probably wouldn't bother. 😆

[SpencerKaiser]

Cool, thanks for the clarification!

Well it really is a crappy situation to be in because you're either dealing with a warning 24/7 or losing type safety (and could potentially forget to include a necessary dependency) by ignoring it. I'll give it a shot and see how it goes 🙂

[edemagbenyo]

Has anyone found a solution to this problem?

[eybel]

Well, Infinite loop here when ading router to my useEffect dependency. I have a router.push(....) inside my useEffect, and eslint keeps asking me to add the depenndency router, when I do, I get infinite loop (fetching API constantly) I had to applied the " // eslint-disable-next-line react-hooks/exhaustive-deps" rule in order for the warning to go away... App works fine now...

This Es-lint thing, makes things harder sometimes and It makes you beliece you need to re-factor the code... that makes you waste time

[icyJoseph]

use the Router instance instead, import Router from 'next/router';

[jackblackCH]

what if use e.g router.pathname and router.push in the useEffect?

[eybel]

@icyJoseph that worked! I have used 'Router.push(......' inside my useEffect() and I removed router from the dependency.

Can you tell me what the difference is? Why now it works with import Router from 'next/router'; instead of import { useRouter} from 'next/router'; ?

I would appreciate that.

use the Router instance instead, import Router from 'next/router';

@jackblackCH I had the following code, but if I added 'router' as a dependency, it kept triggering my URL change. Now with what @icyJoseph proposed its not doing that but I am not sure why.

Code I had before:
import { useRouter } from "next/router";

inside use effect =>
router.push({pathname: 'value', query: 'value'}, undefined, {scroll: false});

Code I have now:
import Router from "next/router";

inside use effect =>
Router.push({pathname: 'value', query: 'value'}, undefined, {scroll: false});

[icyJoseph]

A simple thing to do here is to value each use case and then use the proper tool.

If I am at the top level _app component, and have this effect

const router = useRouter();

useEffect(() => {
  // if there's data stay where we are
  if(data) return;
  // otherwise go to no-data route
  router.push("/no-data")
}, [router, data])

Then I am in trouble, as I push to /no-data, the internal representation of router changes, and I'd get yet another run for this effect.

However, in this case, because we don't care about the current location, we just care about data and wish to side-effect a navigation to no-data, we can just reach out for the Router instance import Router from 'next/router'

import Router from 'next/router';

useEffect(() => {
  // if there's data stay where we are
  if(data) return;
  // otherwise go to no-data route
  Router.push("/no-data")
}, [data])

Now this runs, strictly, only when data changes.

Normally, if I have a router.push call in Component A useEffect, the route transition means that the component and its page siblings unmount, so the useEffect won't trigger on because router changed, even if router is included as dependency.

However, for persistent components, those members of a persistent Layout, the story is different, think of a Header to the entire site, which contains useEffect that does router.push based on some data. Constructing a useEffect that takes router as a dependency is not a good idea, because even after the transition this component is present on the page, so you'd be calculating the useEffect again, and again, etc... You'd be forced to put up special guards. For this case, the Router instance is best.

I know this is not optimal, but it is our job as developers to recognise which tool is best for the job at hand. The Router instance is specially important, because you may want to interact with the Router (listen to events for example), outside of React's, at top of modules, or in Class Components.

useRouter is still very useful for things that keep track of route changes, query changes, and so on, it is just a matter of recognising when a side-effect could end up in a runaway effect.

Another way I've seen people run into runaway effects is with default values:

const {data = []} = useDataFetchingTool();
const [, forceRender] = useReducer(x=>!x, false);

useEffect(() => {
 forceRender();
}, [data])

The above is a made-up example, but when the effect runs and does a forceRender, the value of data will be a new value again, so the effect runs yet once again, and again, etc.

[ValentinH]

I like this explanation 👍

Just one point, how to deal with router.push inside a component but that doesn't change the page? For example, when only changing the query or the hash part of the URL ?

[regisrex]

Adding router to the dependecy array is making some tasks start lagging, why ?

I'm trying to sync the URL query param ?section= with the section state.

    const router = useRouter();
    const [section, setSection] = useState<section>("ideas")
    useEffect(() => {
        router.push({
            pathname: '/panel',
            query: { section: section },
        })
    }, [section])

The code snippet above runs well and doesn't cause lags, but at build time, I was warned that I have to add the router in the useEffect dependacy array , so I added it [section , router].

After I added router in the dependency array, the URL query param was changing after an average of 4seconds after the section state has changed, which is really bad .

Someone can help me understand what's causing the problem and how I can fix it.

[SpencerKaiser]

Not perfect, but this should do the trick. The gist is that you use a ref to keep track of whether or not you need to handle the change or if it's a false alarm. Wherever you're using setSection, just set that ref.current to true and then your useEffect will handle things appropriately.

const [section, setSection] = useState('ideas');
const shouldChangePageRef = React.useRef(false);

React.useEffect(() => {
  if (shouldChangePageRef.current) {
    shouldChangePageRef.current = false;
    void router.push({
      pathname: '/panel',
    });
  }
}, [section, router]);

const someOtherActionHandler = () => {
  shouldChangePageRef.current = true;
  setSection('New section');
}

I'm sure you have your reasons for doing the router.push inside a useEffect, but I typically find that the root of the issue is that I'm not just doing that push logic where the user interaction takes place. If you try and move that, it might solve your problem 😄

Edit: Just a heads up, I modified your code because there were a few types I didn't have so I just removed the type on the useState and the query option on the router.push call.

[geryogam]

Your component was stuck in a render loop because router.push triggered a new render, and useRouter returned a new router object. Since the effect in useEffect depended on router (as it should according to ESLint), it ran again, and router.push started a new cycle.

To prevent it, you should only call router.push if the current URI differs from the target URI. This avoids unnecessary pushes and breaks the loop:

export default function Page() {
  const { user, loading } = useUser()
  const router = useRouter()

  useEffect(() => {
    if (!(user || loading) && router.pathname !== '/login') {  // router.pathname !== '/login' prevents the loop
      router.push('/login')
    }
  }, [user, loading, router])  // router in the dependency array passes ESLint checks

  return <p>Redirecting...</p>
}