Suspense Boundary or `loading.tsx` in Root Layout breaks core features (404, redirects, streaming)

beckerei commented 11 months ago

Link to the code that reproduces this issue

https://codesandbox.io/p/sandbox/top-lvl-suspense-boundary-vtg7qz

To Reproduce

The examples provided is the most minimal to understand the problem. You just wrap your children in the Root of your application in a suspense boundary. In this case it breaks the 404 behaviour, it will also break redirects and streaming documents.

Why exactly are we doing this? We leverage Suspense to control which parts are completely client-side rendered, as in some cases you can not control if a mismatch between server and client happens (e.g. some browser extensions like Password Managers alter the HTML)

We have a Global Navigation that is injected in our applications (not all of them are nextjs apps, never will they be) via SSI and ESI. So we (CDN/Nginx) also alter the HTML that comes from nextjs server, this is no problem until the entire page is thrown away and mounted again on the client. This results in our Global Navigation to be gone and replaced with the ESI/SSI string.

Component we are using.

export class Navbar extends Component {
  shouldComponentUpdate() {
    return false;
  }

  render() {
    return (
      <div
        suppressHydrationWarning
        dangerouslySetInnerHTML={{
          __html: '<!--# include virtual="/fragments/navbar/" -->',
        }}
      />
    );
  }
}

As mentioned in RFC: Server Rendering Errors in React 18

When an error is thrown on the server, React will emit the fallback HTML from the server and then automatically retry rendering on the client. This will affect everything up to the closest boundary above. Similarly, when an error is thrown during hydration on the client, React will discard the server-rendered HTML and revert to a clean client render.

How does it break streaming? The Root page together with the Navbar is loaded, but the loading of the first page is deferred.

I will try to add an example to the repository as well.

Why can't we use a loading.tsx at the root? The behavior is the same.

Current vs. Expected behavior

The page shows the default 404 page, but the status code is 200. I expect the status code to be 404.
Visit via /redirect you can see that the redirects happens with status 200, after it has redirect it reloads the page. I expect a redirect with 307, without additional page reload.

Verify canary release

[X] I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
  Platform: linux
  Arch: x64
  Version: #1 SMP PREEMPT_DYNAMIC Sun Aug  6 20:05:33 UTC 2023
Binaries:
  Node: 20.9.0
  npm: 9.8.1
  Yarn: 1.22.19
  pnpm: 8.10.2
Relevant Packages:
  next: 14.0.5-canary.7
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.1.3
Next.js Config:
  output: N/A

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

App Router

Additional context

No response

zzincist commented 10 months ago

Just for reference for anyone using segment analytics.

This bug breaks this example: https://github.com/vercel/next.js/tree/canary/examples/with-segment-analytics/app

Bandaid solution: Wrap the app in another Layout.tsx and move the <Analytics /> component to that layout instead.

nitinTJ commented 8 months ago

What is the update on this issue? I was facing the same issue and I removed the suspense boundary and loading across my app and the 404 pages status code were fixed but then the LCP of my app has increased. Any help will be appreciated.

irigyano commented 7 months ago

The problem still exists in version 14.2.0-canary.62. When the page with loading.tsx redirects to another page, the status code remains at 200. However, upon removing the loading, it functions correctly with a status code of 307.

erwanriou commented 6 months ago

Same here, this is quite annoying.

Edit by maintainer bot: Comment was automatically minimized because it was considered unhelpful. (If you think this was by mistake, let us know). Please only comment if it adds context to the issue. If you want to express that you have the same problem, use the upvote 👍 on the issue description or subscribe to the issue for updates. Thanks!

vercel / next.js