react-router

react-router copied to clipboard

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.

Yeah same issue as https://github.com/remix-run/react-router/issues/7460

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: https://github.com/remix-run/react-router/blob/0f9435a8134b2b5dddfd716a18d17aefe4461fe1/packages/react-router/lib/hooks.tsx#L156

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.

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?

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.

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)

@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.

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

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

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?

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

No I am already using memoization. I don’t think memoization affects anything about the point in time which the function gets called. It still runs synchronously inside of the child effect. In react children components get their effects processed first, and then the parent. react router is preventing any navigation that occurs prior to the parent effect running. If you were to wrap your call back in a set time out that should be sufficient to delay it until after the parent effect

We wrap navigate in Remix and don't have this problem. Can you make a reproduction in codesandbox or something?

https://codesandbox.io/s/dry-wildflower-gn7iv6?file=/src/App.js open console and observe warning from the codepath pertaining to the bug

Note: the code sandbox / use case may seem contrived, but I would argue its pretty common. In my case, I am trying to call navigate() in something like an onWhatever handler which is defined in the parent.... it's just that the child cannot call it's onWhatever() inside an effect, without the misleading warning (which cancels the navigation). Unlike the warning says, we are not breaking rules of React here (we are not running logic with side effects in the render phase). We're just breaking the heuristic react-router relies on (we're just running in a child effect rather than a parent effect).

No I am already using memoization. I don’t think memoization affects anything about the point in time which the function gets called. It still runs synchronously inside of the child effect. In react children components get their effects processed first, and then the parent. react router is preventing any navigation that occurs prior to the parent effect running. If you were to wrap your call back in a set time out that should be sufficient to delay it until after the parent effect

@ryanflorence Note the following code sandbox: https://codesandbox.io/s/youthful-monad-ef3nry?file=/src/App.js

This issue has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from issues that are unactionable. Please reach out if you have more information for us! 🙂

😆 @ryanflorence this is not resolved 🤷🏻‍♂️

I am not sure why this is tagged as needs-response. Can we consider re-opening this?

EDIT: apparently me commenting fixed it

Need this fixed. I'm trying to upgrade to react-router v6. In one of my components I would like to immediately update the url params for the page in certain cases (users like the url params to update as they change config on the page so that they can share the links afterwards). Attempting to update the url params (via navigate) in a useEffect causes this warning - and it fails to update the url parameters! I will use the setTimeout hack as a work-around for now, however.

I am having an issue with this too. But mostly when using React 18 / react-testing-library v13.3 for unit testing via a memory router. The same unit test worked fine in React 17 / testing library 12.x. Only after an upgrade seeing these issues.

My code is very similar to the one mentioned here https://github.com/remix-run/react-router/issues/8809#issuecomment-1152585637. Only difference is I am calling the navigate function onClick of a button or a link. Navigation fails to happen because of this line if (!activeRef.current) return; because for some reason (useEffects not running in the intended order that react-router expects) at the time of execution activeRef.current is false.

Also want to echo what @mastercactapus mentioned about the warning and silently preventing a navigate. Not sure what was the reasoning behind this as it is hard to decipher what just happened. If it is indeed not desired, it should be an error with better wording. If it is just a warning, navigation should be allowed?

Additionally, if navigates should happen only in effects, should the navigate function now return a promise that folks await on?

@ryanflorence sorry to ping again but its been several months. We promptly provided the requested reproduction code sandboxes. Can you confirm if you were able to replicate the same on your end? Is react-router still actively maintained? Is there another active maintainer that would be better to tag here?

@joshribakoff-sm I was able to reproduce and resolve the issues in theory, but there's a hold up getting approval to sign their CLA, I've been asked for a corporate CLA instead of an individual CLA but one hasn't been provided. I've got the open pull request below although it looks like it has merge conflicts now.

https://github.com/remix-run/react-router/pull/8929

I'll go back to our open source community and see if I can get approval to do an individual CLA contribution. It's necessary because I put this fix together with company time, because we experienced the same issue.

Thanks for the update @42shadow42 - I don't see you signed the CLA yet. I took a look at the repo's CLA and it seems to be applicable to any legal entity:

"You" (or "Your") shall mean the copyright owner or legal entity authorized by the copyright owner that is making this Agreement with Remix.

I'm not sure why your employer would have an issue with the CLA that has been provided.

Thanks for the PR @42shadow42. Removal of the flag helps! But the warning will likely come back, if the child component also uses useLayoutEffect() to trigger the navigate for whatever reason. Because of the hierarchy. So maybe we should additionally mention in the warning message to only navigate in useEffect or async stuff like clicks - not in renders, not in useLayoutEffects?

Thanks for the update @42shadow42 - I don't see you signed the CLA yet. I took a look at the repo's CLA and it seems to be applicable to any legal entity:

"You" (or "Your") shall mean the copyright owner or legal entity authorized by the copyright owner that is making this Agreement with Remix.

I'm not sure why your employer would have an issue with the CLA that has been provided.

Because we were encountering the issue as well and I fixed it on company time, and because of that I interpret them to be the copyright owner? They typically approve these things is my understanding but got hung up the wording that there was a corporate CLA, where remix-run reports having none.

BTW @42shadow42 probably should change the git commit message to say fix:... so that release bot knows it is a patch release fix. Unless ofc someone else needs to do that

Is there any progress on this issue? This bug currently breaks all our tests meaning we can not upgrade to React Router 6.

It looks like the PR https://github.com/remix-run/react-router/pull/8929 has been reviewed but has remained unmerged for months now.

Trying to contribute to open source is frustrating when project maintainers drop the ball like this. If the project is being abandoned please at least let us know

@joshribakoff react-router is far from being an abandoned project. Maintainers got busy with other stuff, that's it. In the meantime, you could apply the changes from the open PR in a fork, or using patch-package until someone from the team have the time to review the pull request.

Patience is key when trying to contribute to OSS.