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

App Router: External redirect causes hundreds of canceled network requests, only works because of React's infinite loop failsafe #48438

Closed
1 task done
sophiebits opened this issue Apr 15, 2023 · 3 comments · Fixed by #49058
Labels
area: app App directory (appDir: true) bug Issue was opened via the bug report template. Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@sophiebits
Copy link
Contributor

sophiebits commented Apr 15, 2023

  • I verified that the issue exists in the latest Next.js canary release
Environment information

On StackBlitz:

❯ next info

    Operating System:
      Platform: linux
      Arch: x64
      Version: Ubuntu 20.04.0 LTS Sat Apr 15 2023 16:22:47 GMT-0700 (Pacific Daylight Time)
    Binaries:
      Node: 16.14.2
      npm: 7.17.0
      Yarn: 1.22.19
      pnpm: 8.2.0
    Relevant packages:
      next: 13.3.1-canary.8
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0

Which area(s) of Next.js are affected? (leave empty if unsure)

App directory (appDir: true), Routing (next/router, next/navigation, next/link)

Link to the code that reproduces this issue

https://stackblitz.com/edit/vercel-next-js-xw3kie?file=app%2Ftest%2Fpage.tsx

To Reproduce

Call redirect() with an external URL from a server component, such that client JS handles the redirect. (Not sure on the exact conditions that trigger this but I got it to work in the repro with one level of route nesting and a 300ms sleep.)

You can repro it on the linked StackBlitz by opening /test in a new browser tab (eg: for me it was https://vercelnextjsxw3kie-1hzg--3000--c8358679.local-credentialless.webcontainer.io/test). Before navigating to that URL, you'll probably want devtools Console and Network open with "Preserve log" enabled in both to see the issue.

Describe the Bug

It does do the redirect successfully, but:

Note in the Network tab that there are literally hundreds of requests to the destination which all get canceled, until one finally goes through:
image
image

In the Console tab, you'll see errors from React about infinite loop, plus one from Chrome about it throttling navigations:
image

This is bad for performance (in my testing, costs about 1 second!) and also confusing as a developer.

Expected Behavior

I expect the redirect to happen without all of these extra requests.

I traced the root cause to how HandleRedirect in redirect-boundary.tsx is implemented to handle the redirect error object. It calls router.replace() and then resets its parent RedirectErrorBoundary once that route transition finishes:

React.startTransition(() => {
router.replace(redirect, {})
reset()
})

For internal navigations, this works fine (?) since the router.replace() state update does not finish until the navigation is complete, so the reset() does not get called until then.

For external navigations, router.replace() succeeds immediately, calling location.replace in a useEffect (based on pushRef.mpaNavigation being set). But that location.replace doesn't prevent further in-page JS from running, so reset() is called, so it again tries to render the original content which redirects, which once again triggers getDerivedStateFromError. Ad infinitum.

Which browser are you using? (if relevant)

I've been testing using Chrome

@sophiebits sophiebits added the bug Issue was opened via the bug report template. label Apr 15, 2023
@github-actions github-actions bot added area: app App directory (appDir: true) Navigation Related to Next.js linking (e.g., <Link>) and navigation. labels Apr 15, 2023
@sophiebits
Copy link
Contributor Author

Possibly related to #48309?

@sophiebits
Copy link
Contributor Author

Another quirk of the MPA-fallback navigation behavior as currently implemented: usePathname and useSearchParams return the pathname and search params of the to-be-redirected-to page:

// page.js
"use client";

import { useSearchParams, useRouter, usePathname } from "next/navigation";

export default function Home() {
  let router = useRouter();
  let pathname = usePathname();
  let searchParams = useSearchParams().toString();
  if (searchParams) {
    // You wouldn't really expect this to run at all, but it alerts
    // '/stuff?abc=123'
    alert(pathname + '?' + searchParams);
  }
  return (
    <button
      onClick={() => {
        router.push("https://www.example.com/stuff?abc=123");
      }}
    >
      go
    </button>
  );
}

sophiebits added a commit to sophiebits/next.js that referenced this issue May 1, 2023
Not 100% convinced that this is the correct fix but it's the best I can think of.

Previously, we would sometimes call location.assign()/.replace() hundreds of times (or more) as I described in vercel#48438 and vercel#48309 (comment). Sometimes this would just make things slow but the navigation would eventually succeed; sometimes this would just hang the browser.

Now we trigger it only once (or—for a reason I don't quite understand—twice in dev, as you can see in the test) and never commit that render.

This also fixes the bug I mentioned in vercel#48438 (comment) where usePathname() and useSearchParams() would return the page we are navigating to (even if that's an external page wholly unrelated to our site!).

Fixes vercel#48309, fixes vercel#48438.
timneutkens pushed a commit that referenced this issue May 3, 2023
Not 100% convinced that this is the correct fix but it's the best I can
think of.

Previously, we would sometimes call location.assign()/.replace()
hundreds of times (or more) as I described in
#48438 and
#48309 (comment).
Sometimes this would just make things slow but the navigation would
eventually succeed; sometimes this would just hang the browser.

Now we trigger it only once (or—for a reason I don't quite
understand—twice in dev, as you can see in the test) and never commit
that render.

This also fixes the bug I mentioned in
#48438 (comment)
where usePathname() and useSearchParams() would return the page we are
navigating to (even if that's an external page wholly unrelated to our
site!).

Fixes #48309, fixes #48438.

link NEXT-1028

Co-authored-by: Jimmy Lai <laijimmy0@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

This closed issue has been automatically locked because it had no new activity for a month. 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 Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: app App directory (appDir: true) bug Issue was opened via the bug report template. Navigation Related to Next.js linking (e.g., <Link>) and navigation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant