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

Calling redirect from a React Server Component to an intercepted route causes infinite rerendering loop #61341

Closed
denexapp opened this issue Jan 29, 2024 · 11 comments · Fixed by #63607
Labels
bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@denexapp
Copy link

denexapp commented Jan 29, 2024

Link to the code that reproduces this issue

https://github.com/denexapp/redirect-from-rsc-to-intercepted-route-bug

To Reproduce

  1. Start the dev server
  2. Open the main page and click on the Link
  3. Wait for 30 seconds
  4. Watch the terminal and the browser console for a list of errors / rerender messages

Current vs. Expected behavior

Expected behaviour:

  • A component from the intercepted route renders once

Current behaviour:

  • Component gets rerendered in a loop
  • Navigation works incorrectly, causing weird errors and sometimes crashing the app

Provide environment information

Operating System:
  Platform: linux
  Arch: x64
  Version: #18~22.04.1-Ubuntu SMP Tue Nov 21 19:25:02 UTC 2023
Binaries:
  Node: 20.11.0
  npm: 10.2.4
  Yarn: 1.22.19
  pnpm: 8.14.1
Relevant Packages:
  next: 14.1.1-canary.20 // Latest available version is detected (14.1.1-canary.20).
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.3.3
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

App Router, Routing (next/router, next/navigation, next/link)

Which stage(s) are affected? (Select all that apply)

next dev (local), next start (local), Vercel (Deployed)

Additional context

The issue occurs with:

  • with babel / without babel
  • on mac os / on github codespaces
  • with turbopack / without turbopack
  • on canary (14.1.1-canary.20) / on stable (14.1.0)
  • with bun / with node
  • locally / on Vercel
@denexapp denexapp added the bug Issue was opened via the bug report template. label Jan 29, 2024
@github-actions github-actions bot added the Navigation Related to Next.js linking (e.g., <Link>) and navigation. label Jan 29, 2024
@xvvvyz
Copy link

xvvvyz commented Jan 30, 2024

Client component redirect has the same issue in my experience:

  1. Navigate to an intercepting route
  2. Navigate to another intercepting route from the previous one
  3. router.back() + router.refresh()
  4. Infinite rerendering loop

Removing the loading.tsx file for the parallel route modal fixes it... but...

Feels related to #61117

@chungweileong94
Copy link
Contributor

Feels related to #61117

Hmm, not sure if they are related. I didn't actually get any errors in my console in #61117, instead I get endless network request instead.

But still, something must be went wrong in the parallel/intercepted routes in v14.1.

@xvvvyz
Copy link

xvvvyz commented Jan 31, 2024

Hmm, not sure if they are related. I didn't actually get any errors in my console in #61117

I'm not seeing errors either with the steps described above actually. It just gets stuck on loading.tsx with infinite requests.

@634750802
Copy link

Same problem with Vercel edge config.

@634750802
Copy link

will #61687 closes this?

@denexapp
Copy link
Author

denexapp commented Feb 6, 2024

I don't know how next works under the hood. To me, it does look similar but not related. Let's see when these changes land to canary

@Cybermb
Copy link

Cybermb commented Feb 10, 2024

I have the same problem in my application using next 14.1.0.
There is a page at /admin which all it does is call redirect("/admin/users") (very similarly to your example). Navigating to /admin using a link for a first time will cause infinite re-render loop. I have tried these server side redirect options: redirect(), permamentRedirect(), redirects in next.config.js. All of them causes same issue, even in production mode but without console warnings. Same happens with next@canary, next@14.0, next@13.5

app-index.tsx:25 Warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.
    at HandleRedirect
    ...

@Cybermb
Copy link

Cybermb commented Feb 13, 2024

After some investigation I have found out that this is not a nextjs bug.
In your example the issue is that you have named your folder (.)photos. Renaming to just photos seem to eliminate the issue.
As for my case, it seem that I had a bug in route layout, where redirect("/admin") function was outside desired condition check. This is why navigating to /admin would cause loop 😅

@denexapp
Copy link
Author

The (.) naming is a required convention for Intercepting routes (1 subtree renders instead of another subtree). Without it it would be regular parallel routes (2 subtrees render at the same time).

Provided example is based on the official Vercel Nextgram example which uses the (.) convention as well, I did the minimum changes to reproduce the issue.

There's a chance that i have a bug in my code, but honestly I wish it was true, cause it is causing me a lot of pain to avoid the bug in a real app.

@Cybermb
Copy link

Cybermb commented Feb 13, 2024

My apologies. I did not know about these advanced routing features.
In context of example, it seems that navigating to /redirectToModal does intercept route correctly. I think the problem is that this route does not render anything, but then intercepts the modal, which is rendered correctly, but page still is trying to redirect, causing the loop? In the docs it says that interception overlays data on current context, but since nothing is rendered, there is no context. Not sure about your intention with this redirect, but if:

  1. You want to intercept route, then you could render client component, that redirects to the desired page.
  2. You want to render only the photo, without overlaying page, then you need to find a way to stop route intercepting on route where you want to redirect.

timneutkens pushed a commit that referenced this issue Mar 28, 2024
### What
When a parallel segment in the current router tree is no longer "active"
during a soft navigation (ie, no longer matches a page component on the
particular route), it remains on-screen until the page is refreshed, at
which point it would switch to rendering the `default.tsx` component.
However, when revalidating the router cache via `router.refresh`, or
when a server action finishes & refreshes the router cache, this would
trigger the "hard refresh" behavior. This would have the unintended
consequence of a 404 being triggered (which is the default behavior of
`default.tsx`) or inactive segments disappearing unexpectedly.

### Why
When the router cache is refreshed, it currently fetches new data for
the page by fetching from the current URL the user is on. This means
that the server will never respond with the data it needs if the segment
wasn't "activated" via the URL we're fetching from, as it came from
someplace else. Instead, the server will give us data for the
`default.tsx` component, which we don't want to render when doing a soft
refresh.

### How
This updates the `FlightRouterState` to encode information about the URL
that caused the segment to become active. That way, when some sort of
revalidation event takes place, we can both refresh the data for the
current URL (existing handling), and recursively refetch segment data
for anything that was still present in the tree but requires fetching
from a different URL. We patch this new data into the tree before
committing the final `CacheNode` to the router.

**Note**: I re-used the existing `refresh` and `url` arguments in
`FlightRouterState` to avoid introducing more options to this data
structure that is already a bit tricky to work with. Initially I was
going to re-use `"refetch"` as-is, which seemed to work ok, but I'm
worried about potential implications of this considering they have
different semantics. In an abundance of caution, I added a new marker
type ("`refresh`", alternative suggestions welcome).

This has some trade-offs: namely, if there are a lot of different
segments that are in this stale state that require data from different
URLs, the refresh is going to be blocked while we fetch all of these
segments. Having to do a separate round-trip for each of these segments
could be expensive. In an ideal world, we'd be able to enumerate the
segments we'd want to refetch and where they came from, so it could be
handled in a single round-trip. There are some ideas on how to improve
per-segment fetching which are out of scope of this PR. However, due to
the implicit contract that `middleware.ts` creates with URLs, we still
need to identify these resources by URLs.

Fixes #60815
Fixes #60950
Fixes #51711
Fixes #51714
Fixes #58715
Fixes #60948
Fixes #62213
Fixes #61341

Closes NEXT-1845
Closes NEXT-2030
ztanner added a commit that referenced this issue Mar 28, 2024
### What
When a parallel segment in the current router tree is no longer "active"
during a soft navigation (ie, no longer matches a page component on the
particular route), it remains on-screen until the page is refreshed, at
which point it would switch to rendering the `default.tsx` component.
However, when revalidating the router cache via `router.refresh`, or
when a server action finishes & refreshes the router cache, this would
trigger the "hard refresh" behavior. This would have the unintended
consequence of a 404 being triggered (which is the default behavior of
`default.tsx`) or inactive segments disappearing unexpectedly.

### Why
When the router cache is refreshed, it currently fetches new data for
the page by fetching from the current URL the user is on. This means
that the server will never respond with the data it needs if the segment
wasn't "activated" via the URL we're fetching from, as it came from
someplace else. Instead, the server will give us data for the
`default.tsx` component, which we don't want to render when doing a soft
refresh.

### How
This updates the `FlightRouterState` to encode information about the URL
that caused the segment to become active. That way, when some sort of
revalidation event takes place, we can both refresh the data for the
current URL (existing handling), and recursively refetch segment data
for anything that was still present in the tree but requires fetching
from a different URL. We patch this new data into the tree before
committing the final `CacheNode` to the router.

**Note**: I re-used the existing `refresh` and `url` arguments in
`FlightRouterState` to avoid introducing more options to this data
structure that is already a bit tricky to work with. Initially I was
going to re-use `"refetch"` as-is, which seemed to work ok, but I'm
worried about potential implications of this considering they have
different semantics. In an abundance of caution, I added a new marker
type ("`refresh`", alternative suggestions welcome).

This has some trade-offs: namely, if there are a lot of different
segments that are in this stale state that require data from different
URLs, the refresh is going to be blocked while we fetch all of these
segments. Having to do a separate round-trip for each of these segments
could be expensive. In an ideal world, we'd be able to enumerate the
segments we'd want to refetch and where they came from, so it could be
handled in a single round-trip. There are some ideas on how to improve
per-segment fetching which are out of scope of this PR. However, due to
the implicit contract that `middleware.ts` creates with URLs, we still
need to identify these resources by URLs.

Fixes #60815
Fixes #60950
Fixes #51711
Fixes #51714
Fixes #58715
Fixes #60948
Fixes #62213
Fixes #61341

Closes NEXT-1845
Closes NEXT-2030
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants