[Bug]: Invalid useEffect warning

[richardgarnier]

What version of React Router are you using?

6.3.0

Steps to Reproduce

Run the following code:

function Child(props) {
  useEffect(() => {
    props.wrappedNavigate('./otherRoute')
  }, []);
  return <></>
}

function Parent() {
  const navigate = useNavigate();
  const wrappedNavigate = (to) => {
    navigate(to);
  }
  
  return <Child wrappedNavigate={wrappedNavigate} />
}

Expected Behavior

Navigate works and performs navigation.

Actual Behavior

You should call navigate() in a React.useEffect(), not when your component is first rendered. is logged into the console.
This is due to the fact that the useEffect of the Child component is executed before the useEffect of the useNavigate, which causes invalid protection on navigation.

[joshribakoff]

Yeah same issue as #7460

[mastercactapus]

Came across this while looking into several failing tests. It appears to cause our click events to be lost when running in CI (or locally with CPU throttling) if the click is performed immediately after rendering.

Maybe useLayoutEffect would be a better option here:

react-router/packages/react-router/lib/hooks.tsx

Line 156 in 0f9435a

React.useEffect(() => {

useEffect runs in the next tick+, not synchronously, so there is a time after everything is rendered and flushed to DOM where a link is rendered and clickable yet this check incorrectly throws away the call to navigate.

Haven't found a reliable workaround yet and editing the library isn't an option for us.

As another suggestion, it might be better to throw an error if it's impossible to continue, rather than silently break things. If it's not a hard requirement to abort navigation, log the warning that unexpected things may happen but continue navigation instead of the following return. Production-mode builds will either work or fail with an error that way.

[42shadow42]

It looks like this was probably introduced based on decisions made here:

If I understand this correctly we can't synchronously navigate in any component render because it causes state updates in other components via this event handler which passes in the state directly.

While I'm no expert here, I think rather than requiring navigation to be triggered after the componentDidMount useEffect indicated in the post above. an alternative solution would be making navigation an asynchronous action. I imagine if navigation requests triggered a setTimeout it would cause navigation to occur after all the useEffects have resolved as demonstrated by this experiment.

By introducing the setTimeout behavior here in the initialization of the navigate function, I think it would eliminate the need for render checking and allow us to safely navigate at any point in the react lifecycle.

Thoughts?

[42shadow42]

Thinking about it further I think useLayoutEffect option proposed by @mastercactapus makes more sense. It doesn't require the use of setTimeout, which is typically discouraged and can sometimes cause unit testing problems. In theory using useLayoutEffect will ensure the ref is updated before any useEffects get called including child components. This resolves the concern from the opening post. Because useLayoutEffect is also executed prior to flushing the updates to the page, it also becomes impossible for test automation to click on elements before the useEffects get executed.

[joshribakoff]

I think the solution is to remove the overzealous check

navigating isn’t a layout effect (depending on your use case), and normal effects don’t run in the render phase, so the previous few comments I do not agree with

a 1ms setTimeout also works fine and doesn’t cause issues with unit testing so long as you’re not doing something naive like waiting 2ms in your test and expecting the URL to update (you should already be using waitForExpect or waitFor in react-testing-library for effects)

[42shadow42]

@joshribakoff a 1ms timeout would still have a race condition in this case, a 0ms timeout may work though. As for the navigation being a layout effect you are right it's not. Layout effects happen right after the render essentially as part of the render flow. As such they shouldn't be navigating. I believe the render and layout effects happen synchronously and don't allow for any code to be executed between. If you were to attempt navigation in a layout effect in my PR you would get a warning. I believe if you did this during a layout effect you would see similar behavior to the reason to they made the change except you would get 2 warning instead of 1. The second would theoretically explain the root cause of the first which could of been obscure.

[joshribakoff]

Both a 0ms timeout and 1ms can take longer than 1ms, the 2nd argument passed to setTimeout is just a minimum delay.

The aforementioned testing utilities (waitFor) are capable of waiting until side effects have occurred by repeatedly making observations by running your callback until the assertion passes, so your tests do not need to wait for an (arbitrary) fixed amount of time.

anyway, I think useLayoutEffect is also a valid way to workaround the issue, but ideally the author of this library could actually chime in here or remove the overzealous warning from their library. Then we don’t need any timeout or layout effect workarounds. It seems non ideal for the router to be this coupled to the structure of the tree and the react lifecycle

[ryanflorence]

I'm not sure why we did that check, looking into it.

[ryanflorence]

The "correct" React thing to do here is to wrap that function in a useCallback.

function Child(props) {
  useEffect(() => {
    props.wrappedNavigate("./otherRoute");
  }, [props.wrappedNavigate]);
  return <></>;
}

function Parent() {
  const navigate = useNavigate();
  const wrappedNavigate = useCallback(
    (to) => {
      navigate(to);
    },
    [navigate]
  );

  return <Child wrappedNavigate={wrappedNavigate} />;
}

Does the problem persist if you do it this way?

[ryanflorence]

Also, can somebody make a codesandbox that reproduces this issue? I can't reproduce it.