Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: v6.12.1 Navigating and changing pages does not work. No errors in console #10579

Closed
tipsxBase opened this issue Jun 9, 2023 · 75 comments · Fixed by #10588
Closed

[Bug]: v6.12.1 Navigating and changing pages does not work. No errors in console #10579

tipsxBase opened this issue Jun 9, 2023 · 75 comments · Fixed by #10588
Labels

Comments

@tipsxBase
Copy link

What version of React Router are you using?

6.12.1

Steps to Reproduce

image

I am encountering an issue in my React 18 environment where the setStateImpl method is not being called. I have thoroughly reviewed my code and ensured that I am using React version 18. However, despite my expectations, the setStateImpl method does not seem to be invoked.

Expected Behavior

I expect the setStateImpl method to be called and perform the state update as intended.

Actual Behavior

I am facing an issue after upgrading my environment to React 18 and react-router-dom v6.12.1. Initially, my pages were rendered correctly when I was using react-router-dom v6.10.0. However, after upgrading to v6.12.1, I encountered a white screen issue. Upon debugging the code in the browser console, I noticed that the setStateImpl method is not being called at the specific location within the RouterProvider component in react-router/lib/component.ts, which is causing the white screen to appear.

image

@tipsxBase tipsxBase added the bug label Jun 9, 2023
@krisgerhard
Copy link

+1 with React 18

6.11.2
Everything works as expected
6.12.0
Navigating and changing pages works.
Had an issue with changing route inside the same React Component. For example /page -> /page/1
componentDidUpdate did not receive the parameter 1. Thought it's an issue with my implementation and did not dig deeper.
6.12.1
Navigating and changing pages does not work. Brower URI changes but pages are not loaded. No errors in console.

@BlackEyeByLee
Copy link

BlackEyeByLee commented Jun 9, 2023

+1 with React 18

Try the following example, 'useNavigate' doesn't work

codesandbox

@tipsxBase tipsxBase changed the title [Bug]: [Bug]: v6.12.1 Navigating and changing pages does not work. Browser URI changes but pages are not loaded. No errors in console Jun 9, 2023
@tipsxBase tipsxBase changed the title [Bug]: v6.12.1 Navigating and changing pages does not work. Browser URI changes but pages are not loaded. No errors in console [Bug]: v6.12.1 Navigating and changing pages does not work. No errors in console Jun 9, 2023
@ns4style
Copy link

ns4style commented Jun 9, 2023

got same behavior after upgrading to 6.12.1.
webpack dev bundle is ok but navigation of production bundle doesn't work.
After small investigation I got that any minimizer plugin is causing this strange behavior. (I tried Terser and Esbuild minimizer).

@aerialist7
Copy link

aerialist7 commented Jun 9, 2023

+1

It works fine in development mode but doesn't work in production.
Seems that problem is in minification.

Environment:

  • webpack@5.86.0
  • webpack-cli@5.1.4
  • terser-webpack-plugin@5.3.9
  • react-router@6.12.1
  • react-router-dom@6.12.1
  • react@18.2.0
  • react-dom@18.2.0

@simPod
Copy link

simPod commented Jun 9, 2023

Downgrading react-router-dom to v6.21.0 helps. Did not have to downgrade react-router.

@birdofpreyru
Copy link

birdofpreyru commented Jun 9, 2023

Same problem — in my existing project everything worked fine with react-router-dom@6.11.2 (might have work with 6.12.0 as well — not sure now); after an upgrade to v6.12.1 the navigation via Link elements broke in a peculiar way:

  • No links work in my project's production build, no errors in console / no errors captured by a remote error tracking system (Sentry);
  • Links still work fine in development build, running locally.
  • Links still work both in production and development build of my simple test project. The difference between it, and my real project — the test one barely uses a few Suspenses for code splitting, and links still work across them; the real project heavily relies on suspenses to code-split different parts of the app, and thus I guess the way 6.12.0 messed up with the transitions did not account for complex projects with many nested suspenses, and to be safe it is probably better to downgrade to v6.11.2 until the root cause of the issue is fixed in react-router-dom.

@Razviar
Copy link

Razviar commented Jun 9, 2023

Yeah, same here, in prod "react": "18.2.0", after upgrade to "react-router-dom": "6.12.1" all navigation got broken: router just ignores any navigate actions, but URI in browser changes. Downgrade to 6.11.2 fixes the issue.

For dev enviroment everything is working fine, which is weird.

@muresanandrei
Copy link

Same here holy it seems something has been broken

@UntilGone
Copy link

UntilGone commented Jun 9, 2023

Same here.react in 18.2.0 and react-router-dom in 6.12.0. In development mode it works ok, but in production when hash change, page doesn’t render. And downgrade works for me,

@brophdawg11
Copy link
Contributor

Looking into the reproduction in #10579 (comment), I believe the root cause in that example seems to be calling React.lazy inside the render cycle, which results in a new Promise being created on each render. Internally React deals with thrown promises by adding custom fields but if it's a net-new promise each time it will never see those custom fields and get's itself into an infinite Suspense loop.

Lifting those React.lazy calls out of the render cycle fixes the issue: https://codesandbox.io/s/dawn-rgb-yzvtcw?file=/src/App.js. Note I also changed LazyLoad(Comp) to <LazyLoad Comp={...} /> since I don't think you're technically supposed to be calling React components and normal functions.

Does anyone else have an alternate reproduction we could look into to see if there is a core issue with how we're using startTransition or if this is just a nuance about how React 18 handles concurrent rendering?

In some cases, we think that built-in React behavior might be the source of confusion in that with transitions they don't re-show already resolved Suspense boundaries, so in some cases you can also add a key to your Suspense boundary if it's not re-showing. See #10568.

@birdofpreyru
Copy link

@brophdawg11 While the new React docs say indeed that lazy() should not be used inside the render function of another component (https://react.dev/reference/react/lazy#my-lazy-components-state-gets-reset-unexpectedly) (though, still don't forbid it completely); the legacy docs (which were the official docs until very recently) never told so: https://legacy.reactjs.org/docs/code-splitting.html#reactlazy . Thus, I guess, lots of people use it the way which is incompatible with changes you did in react-router-dom. IMHO, ideally, you should try to correct your changes to ensure they don't break stuff when lazy() is used inside another component; alternatively, you probably can detect it somehow and throw a hard error in such case (so it always breaks in both prod and dev, and not like now — only in prod, and not in every codebase)?

@brophdawg11
Copy link
Contributor

lots of people use it the way which is incompatible with changes

I agree - this is why I'm asking for more reproductions. That will give is a more complete picture of the ways folks in which are using Suspense features today that don't play nice with startTransition and make sure we come up with the right solution.

Is calling lazy inside render the same root issue in your application? If not, would you be able to provide a reproduction of your scenario?

@birdofpreyru
Copy link

birdofpreyru commented Jun 9, 2023

Is calling lazy inside render the same root issue in your application? If not, would you be able to provide a reproduction of your scenario?

Yeap. I currently use it this way in my library I rely upon across my projects. Funny thing, when I wrote that code, I even left a comment for future myself, that I wasn't sure, from React docs, whether such usage is permitted; but in practice it caused me no troubles till now, so I never revised it.

@Doridian
Copy link

Doridian commented Jun 9, 2023

In the same boat here, 6.12.0 works, 6.12.1 breaks.
I am using lazy in the project, but not inside the render. (Full code is here, but I don't expect anyone to actually dig through this, I'll try and cut it down to a minimal repro: https://github.com/foxCaves/site/blob/chore/update-router/frontend/src/app.tsx#L19 )

@brophdawg11
Copy link
Contributor

In the same boat here, 6.12.0 works, 6.12.1 breaks.

Huh, any chance that's a typo and supposed to say 6.11.0 works? If 6.12.0 -> 6.12.1 is breaking for you a reproduction would be awesome. The only change in 6.12.1 was to avoid a compilation issue for webpack/react@17 - but both of them should be wrapping your router updates in React.startTransition...

@Doridian
Copy link

Doridian commented Jun 9, 2023

In the same boat here, 6.12.0 works, 6.12.1 breaks.

Huh, any chance that's a typo and supposed to say 6.11.0 works? If 6.12.0 -> 6.12.1 is breaking for you a reproduction would be awesome. The only change in 6.12.1 was to avoid a compilation issue for webpack/react@17 - but both of them should be wrapping your router updates in React.startTransition...

Funnily enough, no. I've spent 2 hours yesterday trying to comprehend how a change from a string to a constant would have the ability to break anything.
But, for a matter of fact, this PR here: https://github.com/foxCaves/site/pull/446/files fails the E2E browsertests, while the state before it passes. (And manual testing shows .1 and only .1 breaks navigation).

Sites load, the initial page you loud routes correctly, but clicking any link just changes the address bar and doesn't load the actual page/content.

@jlowcs
Copy link

jlowcs commented Jun 9, 2023

I can confirm that 6.12.1 is the issue on my end as well. 6.12.0 works fine.

I can help investigate but not before Tuesday sadly.

@turansky
Copy link
Contributor

turansky commented Jun 9, 2023

React 18.2.0

We don't use React.lazy at all and have the same problem with links in 6.12.1.
6.12.0 - works fine.

@brophdawg11
Copy link
Contributor

That is ... surprising that the 6.12.0 -> 6.12.1 fix could change any behavior 😂. If anyone is able to provide a simplified reproduction of that it would be super helpful, I'll see if I can reproduce it in the meantime.

While we dig into the underlying React behavior a bit here, if you're running into this, it's best to stay on 6.11 for a bit longer.

So far it seems like this is a bit of a case of the fact that Suspense has been "released" for some time but not fully understood (by us included!) so we're sort of "holding it wrong" in some cases. Now that Suspense is a bit better documented/supported/understood, we're seeing startTransition expose some of those issues.

If you want to try to get your current app working in the interim (and better prepare your code for a fully-concurrent/transition-enabled world):

  • Avoid re-creating net new promises on renders. Either by lifting them out of the component (i.e., call React.lazy() at the top of your module) or by wrapping the promise creation in useMemo. Either of those should fix the issue since React will be able to re-use the same promise on the subsequent concurrent render.
  • If you can't get around the new Promise creation, you can add a key to your Suspense boundary to trigger a new instance which will be allowed to fall back in concurrent mode.

We'll keep digging into these reproductions and figure out the best path forward. Sorry for the hiccup!

@hongz1
Copy link

hongz1 commented Jun 9, 2023

I haven't updated react-router-dom since v6.8.2 and it suddenly had this problem.
I upgraded to v6.12.0 and it worked fine.

@brophdawg11
Copy link
Contributor

brophdawg11 commented Jun 9, 2023

@Doridian Are there any specific links that fail for you? I cloned your site and ran the /frontend app in both dev and prod (with a stubbed window.FOXCAVES_CONFIG) mode on both 6.12.0 and 6.12.1 and didn't have any issues routing between the homepage and /login and /register

@simPod
Copy link

simPod commented Jun 9, 2023 via email

@Doridian
Copy link

Doridian commented Jun 9, 2023

@brophdawg11 Dev mode navigation works. For me navigation only breaks (but reliably so) in a production build.
You have to NODE_ENV=production npm run build. Only then is it broken. (You either have to symlink /static to the build folder or change https://github.com/foxCaves/site/blob/main/frontend/webpack.config.ts#L23 to always be / to be able to easily run this in a simple server like python3 -m http.server)

@Doridian
Copy link

Doridian commented Jun 9, 2023

Interestingly, this only happens with code minifcation enabled. I wonder if this hit a bug in the minifier webpack uses?

@Doridian
Copy link

Doridian commented Jun 9, 2023

Looking at the minified code portion of the startTransition thing:

[...]
const _ = 'startTransition';
function w(e) {
    let { basename: t, children: s, window: l } = e,
        u = o.useRef();
    null == u.current && (u.current = (0, i.lX)({ window: l, v5Compat: !0 }));
    let c = u.current,
        [d, f] = o.useState({ action: c.action, location: c.location }),
        p = o.useCallback(
            (e) => {
                _ in (r || (r = n.t(o, 2))) || f(e);
            },
[...]

It is quite hard to read anything in the minified stuff, but it almost seems as if it misinterprets the x in y ? A : B and gets the order of things wrong? (Judging from the || being inside the thing the in checks)
I'll try playing around with this and see how various things minify...

@brophdawg11 brophdawg11 reopened this Jun 13, 2023
@brophdawg11
Copy link
Contributor

We just released 6.13.0-pre.1 which should address both of the above issues:

  • React.startTransition is no longer used by default. If you wish to enable it, you can pass a future flag to your router component:
    • <BrowserRouter future={{ v7_startTransition: true }}>
    • <RouterProvider router={router} future={{ v7_startTransition: true }} />
  • For those of you opting into the use of React.startTransition, the minification issue should no longer be present and navigation should be functional again in production builds (the issue was causing startTransition/setState not to be called in the minified code)

Please 👍 this comment if you can confirm this resolves your issue, or let us know if you run into any issues.

Assuming nothing comes up we'll aim to have a 6.13.0 stable release out tomorrow. There are no other changes in 6.13.0 from 6.12.0 - we just did a minor version bump due to the introduction of the future.v7_startTransition flag.

@remarkablemark
Copy link

Would it be possible going forward to release a revert patch when a bug is confirmed so that repositories with Dependabot enabled wouldn't accidentally merge the bug?

@RajanKumarGoyal
Copy link

I am also facing the same Problem yesterday, Earlier it was working good. Yesterday I Found out that my routes are not working properly in production mode but its working good in development mode. It was a strange behaviour.

After reading the above thread & spent couple of hours on this, I concluded that you can resolve this "route change but not rendering view" issue from 2 ways.

  • Either remove the lazy loading from your main component (Then routes will render both for 6.12.1 and 6.12.0 as well in Production mode as well)
  • Or Upgrade your react-router-dom package to "6.13.0-pre.1". This will resolve your issue with lazy loading. And wait for "6.13.0" stable version.

Thanks!

@dobladov
Copy link

6.13.0-pre.1 solves the navigations issues for me, while using Suspense and lazy loading.

@ricardasjak
Copy link

"react-router-dom": "^6.3.0" broke it out of nowhere, never ever use default npm version tagging, when minor version can be updated by vendor anytime.

quick fix is to freeze the version:
"react-router-dom": "6.3.0"

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.13.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.13.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11
Copy link
Contributor

Hey folks, this should now be resolved in 6.13.0. Thanks again for bearing with us and helping us triage the issue and apologies for any inconvenience!

As a reminder we would recommend you pin your dependencies in package.json so version bumps are appropriately validated/tested. In an ideal world every patch and minor bump would go smoothly, but with such a large landscape of libraries and bundling approaches it's tough to ensure that nothing ever slips though.

@bluepeter
Copy link

Release 6.12.1 didn't work for many folks (as this issue demonstrates). Release 6.13.0 fixes things, but the release notes for that state:

The tl;dr; is that 6.13.0 is the same as 6.12.0 bue we've moved the usage of React.startTransition behind an opt-in future.v7_startTransition future flag because we found that there are applications in the wild that are currently using Suspense in ways that are incompatible with React.startTransition.

Can someone provide guidance on the "incompatible" ways some of us may be using Suspense, lazy, etc? Right now, we are using the approach pretty much the same as that in https://github.com/remix-run/react-router/blob/dev/examples/lazy-loading/src/App.tsx

It would be good to get some guidance on how we can remedy our code to use startTransition...

@brophdawg11
Copy link
Contributor

Keep reading further in the release notes 😉

Issues usually boil down to creating net-new promises during the render cycle, so if you run into issues when opting into React.startTransition, you should either lift your promise creation out of the render cycle or put it behind a useMemo.

Notice how in the example you link to in the repo, React.lazy() is called at the top-level module scope, thus creating the Promise outside of the render cycle (✅). Compared to the reproduction higher in this issue which is calling React.lazy inside the render cycle, and thus creating a new promise each time (❌).

Another thing to keep in mind is the Suspense behavior to not fallback again after resolving, from the same comment above:

In some cases, we think that built-in React behavior might be the source of confusion in that with transitions they don't re-show already resolved Suspense boundaries, so in some cases you can also add a key to your Suspense boundary if it's not re-showing. See #10568.

@bluepeter
Copy link

Thank you very much for the detailed reply @brophdawg11

@sethlivingston
Copy link

I'm seeing this in 6.14.1, but not in 6.13.0.

@brophdawg11
Copy link
Contributor

@sethlivingston would you mind opening a new issue with reproduction steps?

@siosio34
Copy link

This issue is still reproduced in version 6.15.0.
We are using lazy and suspense and the code below doesn't work.

export default function Redirect({ to }: RedirectProps): JSX.Element {
  const params = useParams()

  const { search } = useLocation()

  const navigate = useNavigate()

  useEffect(() => {
    navigate(
      {
        pathname: generatePath(to, params),
        search,
      },
      { replace: true }
    )
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [])

  return null
}

@brophdawg11
Copy link
Contributor

@siosio34 would you mind opening a new issue with a minimal reproduction?

@conqr2
Copy link

conqr2 commented Aug 30, 2023

We've reproduced this in version 6.11.2. We're using Create-React-App with a BrowserRouter and doing the below raises no logs or errors and has no effect.

function Hook (){
   const navigate = useNavigate();
   
   const longTask = async () => {
      // await 5 second long task
      navigate("/link"); // does nothing
   };
   
   return <button onClick={longTask}>Task</button>;
}

@brophdawg11
Copy link
Contributor

Come on folks, I really don't know how to say this any clearer. Please do not comment on this closed issue with new problems. Please open a new issue with a reproduction in codesandbox, stackblitz, or a github repo. You can link to this issue from the new issue if you think it's a regression of the same type of issue.

@KrzysztofZawisla
Copy link

I know that there is a request to don't reply in this thread, but for everyone like me who struggle with these errors, and don't see any solution on the internet, try to test your code without StrictMode. 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.