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

[NEXT-325] NextJS13 - AppDir - Server redirect happens as fetch not browser redirection #43605

Closed
1 task done
schemburkar opened this issue Dec 1, 2022 · 12 comments · Fixed by #46674
Closed
1 task done
Assignees
Labels
area: app App directory (appDir: true) linear: next Confirmed issue that is tracked by the Next.js team. Navigation Related to Next.js linking (e.g., <Link>) and navigation. Pages Router Related to Pages Router.

Comments

@schemburkar
Copy link
Contributor

schemburkar commented Dec 1, 2022

Verify canary release

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

Provide environment information

Operating System:
  Platform: win32
  Arch: x64
  Version: Windows 10 Home Single Language
Binaries:
  Node: 18.5.0
  npm: N/A
  Yarn: N/A
  pnpm: N/A
Relevant packages:
  next: 13.0.6-canary.3
  eslint-config-next: 13.0.2
  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)

Link to reproduction - Issues with a link to complete (but minimal) reproduction code will be addressed faster

https://github.com/vercel/next.js/files/10133225/next-redirect-demo.zip

To Reproduce

Build & run the sample

next-redirect-demo.zip

yarn install
yarn dev

Navigate to /test/A this should do below:

  • Renders page test/[slug]/page, which redirects to /external
  • /external is a pages route and server redirects (302) to external site google.com

The server redirect is done via fetch and not browser redirection.

image

Describe the Bug

The app has below pages

  • /test/[slug]/page.tsx - using app dir route and server redirect from next/navigation
  • /external - pages route that redirects to external site.

If user enters /external in url, the page correctly does a 302 redirect to external site. This is expected behaviour.

If user enters /test/A in the url, the page also returns a 302, but the redirection happens via fetch. This is not expected behaviour.

Expected Behaviour:
The redirection should be browser redirection and not done via fetch.

To reproduce the bug

Navigate to /test/A this should do below:

  • Renders page test/[slug]/page, which redirects to /external
  • /external is a pages route and server redirects to external site google.com

Expected Behavior

The route /test/A should be browser redirection and not done via fetch.

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

NEXT-325

@schemburkar schemburkar added the bug Issue was opened via the bug report template. label Dec 1, 2022
@balazsorban44 balazsorban44 added area: app App directory (appDir: true) Navigation Related to Next.js linking (e.g., <Link>) and navigation. Pages Router Related to Pages Router. labels Dec 1, 2022
@Fredkiss3
Copy link
Contributor

I think if you want to redirect to an external domain, you could use middleware.ts, as next redirect function is more for redirecting between your routes and server components.

@schemburkar
Copy link
Contributor Author

@Fredkiss3 I have a different opinion. If a redirect is being sent from a page with a 302 status code, it should always be a browser redirect. Not handled via xhr/fetch. It should not matter if its a external domain.

Also there is conflicting behaviour if the /external page is hit via url vs being called by another redirect from app dir. That means this is a bug in either case.

@boehlerlukas
Copy link

@schemburkar I was expecting a server side 302 redirect as well.

@31i0t
Copy link

31i0t commented Dec 8, 2022

Same issue here. The problem manifests similarly with a CORS error. I believe this is unexpected behavior, no? The redirect() docs say the URL can be relative or absolute. Why use an absolute URL for anything that isn't external?

@31i0t
Copy link

31i0t commented Dec 8, 2022

Workaround:
app/settings/page.tsx

import Redirect from "./Redirect"

export default function SettingsPage() {
...
- redirect('https://google.com')
+ return <Redirect />
...
}

app/settings/Redirect.tsx

'use client'

import { useEffect } from 'react'

export default function Redirect() {
  useEffect(() => {
    window.location.replace('https://google.com')
  })
  return null
}

@lucchev
Copy link

lucchev commented Dec 8, 2022

Same issue here with middleware sending an external redirection.

import { NextURL } from 'next/dist/server/web/next-url';
import { NextRequest, NextResponse } from 'next/server';

export function middleware(request: NextRequest) {
  const externalBaseURL = 'https://...';
  return NextResponse.redirect(new NextURL(request.nextUrl.pathname, externalBaseURL));
}

export const config = {
  matcher: '/test'
};

It's interpreted by the Next client as a fetch, not a browser redirection.
So same CORS error...

And I see no way to override Next fetch with mode:"no-cors"

@LGmatrix13
Copy link

Been experiencing this issue still in 13.1. @boehlerlukas’s recommendation for a 302 server side redirect seems like a good idea…

@timneutkens timneutkens added kind: bug linear: next Confirmed issue that is tracked by the Next.js team. and removed bug Issue was opened via the bug report template. labels Dec 29, 2022
@timneutkens timneutkens changed the title NextJS13 - AppDir - Server redirect happens as fetch not browser redirection [NEXT-325] NextJS13 - AppDir - Server redirect happens as fetch not browser redirection Dec 29, 2022
@timneutkens timneutkens self-assigned this Dec 29, 2022
@DaleLJefferson
Copy link

Yes this is really annoying, since the redirect is in a server component I was expecting to see a server side redirect.

This workaround #43605 (comment) worked but it's not very satisfying.

@morgangallant
Copy link

Can confirm that the above-mentioned workaround did work, was having an issue where redirecting to another route (i.e. from /dashboard -> /login) wouldn't render the contents of /login properly, would just show the layout and blank otherwise. Not sure if this is immediately related to this issue or not, but certainly a similar fix — seems very reasonable that redirect() should be implemented as a 302 or something similar to it.

@ChristianIvicevic
Copy link
Contributor

I am experiencing another minor issue with this behavior myself. In my root page I just have the following:

import { unstable_getServerSession } from 'next-auth';
import { redirect } from 'next/navigation';
import { authOptions } from 'pages/api/auth/[...nextauth]';

const Page = async () => {
  const session = await unstable_getServerSession(authOptions);
  redirect(session !== null ? '/lobby' : '/login');
};

export default Page;

The Vercel dashboard of my application shows a white screen and no preview which may be due to a lack of a browser-based redirect. Ironically the redirect works as expected with a classic index file at pages/index.tsx that contains the following code:

import type { GetServerSideProps } from 'next';
import { unstable_getServerSession } from 'next-auth';
import { authOptions } from 'pages/api/auth/[...nextauth]';

const Page = () => null;

export default Page;

export const getServerSideProps: GetServerSideProps = async ({ req, res }) => {
  const session = await unstable_getServerSession(req, res, authOptions);
  return { redirect: { destination: session !== null ? '/lobby' : '/login', permanent: false } };
};

Unlike the use of redirect() the old approach via gSSP takes the environment into consideration.

@timneutkens
Copy link
Member

timneutkens commented Mar 2, 2023

Had a look into this one, it happens because there is a redirect() call to a url that ends up redirecting, fetch() doesn't give us much to deal with this case because you can't get the location header when setting redirect: 'manual' on fetch otherwise this case would have an easy solution.
What we'll have to do instead is always forcing a browser navigation when the fetch for the RSC payload fails.

Update: opened a PR here: #46674

timneutkens added a commit that referenced this issue Mar 4, 2023
Fixes #43605
Fixes NEXT-325
Fixes NEXT-193

This ensures that when the `fetch()` for the RSC payload gets redirected
server-side to a URL that fails CORS (e.g. an external url). This
catches the error and returns the url that was fetches to be
hard-navigated. This also covers other cases like when the host can't be
reached.

Added a note to add a test for this later as it depends on having a
deployment without CORS.

<!--
Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:
-->

## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have a helpful link attached, see
[`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the
feature request has been accepted for implementation before opening a
PR.
- [ ] Related issues linked using `fixes #number`
- [ ]
[e2e](https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have a helpful link attached, see
[`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Documentation / Examples

- [ ] Make sure the linting passes by running `pnpm build && pnpm lint`
- [ ] The "examples guidelines" are followed from [our contributing
doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md)

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 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 Apr 5, 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) linear: next Confirmed issue that is tracked by the Next.js team. Navigation Related to Next.js linking (e.g., <Link>) and navigation. Pages Router Related to Pages Router.
Projects
None yet
Development

Successfully merging a pull request may close this issue.