vercel / next.js

The React Framework
https://nextjs.org
MIT License
126.82k stars 26.95k forks source link

[NEXT-835] Next.js attempts to fetch API route proxy as regular page #37783

Open migueloller opened 2 years ago

migueloller commented 2 years ago

Verify canary release

Provide environment information

    Operating System:
      Platform: darwin
      Arch: arm64
      Version: Darwin Kernel Version 21.4.0: Fri Mar 18 00:46:32 PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T6000
    Binaries:
      Node: 16.15.0
      npm: 8.5.5
      Yarn: 1.22.18
      pnpm: N/A
    Relevant packages:
      next: 12.1.7-canary.40
      react: 18.2.0
      react-dom: 18.2.0

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

When proxying a page using an API route, Next.js will attempt to request the page's JavaScript at the rewrite destination instead of the source page. For example, with the following rewrite setup, a request to http://localhost:3000?proxy=true will return the correct HTML but then Next.js will send a request to http://localhost:3000/_next/static/chunks/pages/api/proxy.js. That request will 404, resulting in a full page reload, which then results in the same behavior, again and again in an infinite loop.

/** @type {import('next').NextConfig} */
const nextConfig = {
  reactStrictMode: true,
  async rewrites() {
    return {
      beforeFiles: [
        {
          source: '/:path(.*)',
          has: [{ type: 'query', key: 'proxy', value: 'true' }],
          destination: '/api/proxy?path=:path',
        },
      ],
    }
  },
}

module.exports = nextConfig

Expected Behavior

The expected behavior is for Next.js to request the page's JS and not to use the rewrite source to determine the URL.

Link to reproduction

https://github.com/migueloller/next-proxy-rewrite-bug

To Reproduce

git clone https://github.com/migueloller/next-proxy-rewrite-bug
cd next-proxy-rewrite-bug
yarn install
yarn dev
open http://localhost:3000?proxy=true

NEXT-835

TommyDew42 commented 2 years ago

Hi! Can i pick this one up?

migueloller commented 2 years ago

@ijjk, the issue was automatically closed but the issue still seems to be present. Instead of an infinite redirect, there's just an error throw now. The request to the non-existing JS file is still there. Are there any plans to support proxying like this so that the page will load and request the source page's JS instead of the non-existing JS for the API route?

ijjk commented 2 years ago

Hi, the above fix is still publishing and will be available in v12.1.7-canary.49 of Next.js, when the release is up please update and give it a try!

migueloller commented 2 years ago

Hi, the above fix is still publishing and will be available in v12.1.7-canary.49 of Next.js, when the release is up please update and give it a try!

🤦🏻 silly me thinking it was already published. Sorry about that! Will test as soon as it's published and report back. Thanks!

migueloller commented 2 years ago

I had a chance to take v12.1.7-canary.49 for a spin in the reproduction. It seems that now there's a hard navigation to the rewrite destination (i.e., the API route), effectively turning the rewrite into a redirect. This will result in the rewrite not being useful since rewriting to a page from an API route will always cause this redirect. This is not quite the expected behavior but I understand if perhaps the expected behavior is not possible.

ijjk commented 2 years ago

Re-opening as it seems we are hard navigating to the rewrite destination incorrectly instead of the initial provided route when the rewrite is detected.

Will continue discussion around alternatives to avoid the hard navigation in slack thread.

TommyDew42 commented 2 years ago

Sorry it took me a while to start looking at the codebase and the investigation work. Seems to me it's not an easy issue to fix since this has started an internal discussion in slack thread. Should I pass back the work to the core vercel team to work on?

wyattjoh commented 2 years ago

We're taking a look into this now!

migueloller commented 2 years ago

Sorry it took me a while to start looking at the codebase and the investigation work. Seems to me it's not an easy issue to fix since this has started an internal discussion in slack thread. Should I pass back the work to the core vercel team to work on?

Sorry for the delayed response. I think it might be doable as long as the new expected behavior is clear. The reproduction should be enough to test the fix. @wyattjoh, do you know what is the final behavior that the Vercel team wants for this fix?

wyattjoh commented 2 years ago

Ultimately I think we want to correct the current behaviour we're seeing now post the #37949 change. Ideally, we want to hard redirect the user to the page that will perform the rewrite, not the destination of the rewrite (essentially a redirect). The edge case shown by the reproduction where the page is actually a proxied Next.js page is something that will have to be handled separately as it relates to the query hydration process.

migueloller commented 2 years ago

Ultimately I think we want to correct the current behaviour we're seeing now post the #37949 change. Ideally, we want to hard redirect the user to the page that will perform the rewrite, not the destination of the rewrite (essentially a redirect). The edge case shown by the reproduction where the page is actually a proxied Next.js page is something that will have to be handled separately as it relates to the query hydration process.

Got it. In that case, I think the old behavior when I opened this issue was basically that: what would happen was a hard-refresh, which is the same as a hard navigation to the rewrite source. The main problem, though, is that if you navigate to the rewrite source, the rewrite will trigger again, and again, and again. So I guess the hard navigation should only happen if you're not already at that page. But then that feels wrong too since it would basically do nothing at that point, ignoring the rewrite.

wyattjoh commented 2 years ago

38340 combined with #37949 does solve a problem though, in that we no longer fetching a JS chunk for pages that have prefix of /api. Additionally, this ensures that any use of <Link /> on a route that is rewritten (or sent directly to) one of these routes results in a hard navigation. These routes typically can't be handled as a client side transition anyways, these changes enable the correct behaviour.

As for handling the redirection loop that's occurring for your proxying use case, that's something that we'll need to handle as a part of the hydration phase. I'd like to keep this issue open as we explore a solution to either providing a more concrete error or direction on how to handle this case.

floriangosse commented 1 year ago

The edge case shown by the reproduction where the page is actually a proxied Next.js page is something that will have to be handled separately as it relates to the query hydration process.

@wyattjoh Is there an issue for that? We actually have the problem that we want to server a page on different path. So we're using rewrites to accomplish that but have the hard navigation error quite often which is quite annoying as it's one of our main pages and client side navigation should work for a good user experience.

Please let me know if I should provide any details about how our setup for reproduction.

lextas commented 1 year ago

Is there any update on this? Or a work-around?

We have a scenario where we host our nextjs app and a backend service on the same url. The frontend makes a call to /auth/user and if that results in a 401 it will redirect to /auth/login (which ends up on the backend service in the kubernetes cluster).

When not logged in, on the redirect, this results in the following error:

Unhandled Promise rejection: Failed to lookup route: /auth/login ; Zone: <root> ; Task: Promise.then ; Value: Error: Failed to lookup route: /auth/login
...

The redirect itself is done using a router.replace('...').

I traced it back to the route-loader but that code has not been changed in years so it seems, so something else might have changed this behavior.

P.s. I'm not sure in which version this "bug" was introduced but I think it kept working till version 13.2.4 (based on git history). After upgrading our e2e tests failed on this unhandled promise.

Current app nextjs version: 13.4.10 (issue still exists).

Happy to provide any additional data / info if I can