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

not-found loses layout #52718

Closed
1 task done
karlhorky opened this issue Jul 15, 2023 · 6 comments · Fixed by #52790 or #52985
Closed
1 task done

not-found loses layout #52718

karlhorky opened this issue Jul 15, 2023 · 6 comments · Fixed by #52790 or #52985
Assignees
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Jul 15, 2023

Verify canary release

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

Provide environment information

Operating System:
      Platform: linux
      Arch: x64
      Version: #22 SMP Tue Jan 10 18:39:00 UTC 2023
    Binaries:
      Node: 16.17.0
      npm: 8.15.0
      Yarn: 1.22.19
      pnpm: 7.1.0
    Relevant Packages:
      next: 13.4.10-canary.8
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 4.9.5
    Next.js Config:
      output: N/A

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

App Router

Link to the code that reproduces this issue or a replay of the bug

https://codesandbox.io/p/sandbox/sleepy-leavitt-q76g5v?file=%2Fapp%2Fnot-found.tsx

To Reproduce

  1. Visit the CodeSandbox above
  2. Navigate to a not-found page using the link or altering the URL
  3. Layout is missing on the not-found page 💥
Kapture.2023-07-14.at.16.42.03.mp4

Describe the Bug

The not-found page no longer receives a layout

Expected Behavior

The not-found page receives a layout

Related issues / PRs

Possibly related to this issue and the PR addressing it:

  1. Issue: [NEXT-1220] layout.js rendering twice in production #49115 by @joshwcomeau
  2. PR: Ensure root layout only render once per request #52589 by @huozhi, reviewed by @shuding

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

NEXT-1449

@karlhorky karlhorky added the bug Issue was opened via the bug report template. label Jul 15, 2023
@karlhorky
Copy link
Contributor Author

To contrast this with the previous behavior, I forked the sandbox above to downgrade to next@13.4.9 (no other changes other than the version number), and header from the layout is working again:

CodeSandbox: https://codesandbox.io/p/sandbox/immutable-thunder-ym59x5?file=%2Fapp%2Fnot-found.tsx%3A1%2C1

Kapture.2023-07-14.at.16.48.53.mp4

@huozhi huozhi added the linear: next Confirmed issue that is tracked by the Next.js team. label Jul 15, 2023
@karlhorky
Copy link
Contributor Author

Potential duplicate issue here:

@huozhi huozhi self-assigned this Jul 18, 2023
ijjk pushed a commit that referenced this issue Jul 20, 2023
### What

This PR changes the flow of not-found rendering process. 

### Why

`not-found.js` was rendered in two ways before:
* 1 is SSR rendering the not-found as 404
* 2 is triggering the error on RSC rendering then the error will be
preserved in inline flight data, on the client it will recover the error
and trigger the proper error boundary.

The solution has been through a jounery:
No top-level not found boundary -> introduce metadata API -> then we
create a top level root not found boundary -> then we delete it due to
duplicated rendering of root layout -> now this

So the solution before this PR is still having a root not found boundary
wrapped in the `AppRouter`, it's being used in a lot of places including
HMR. As we discovered it's doing duplicated rendering of root layout,
then we removed it and it started failing with rendering `not-found` but
missing root layout. In this PR we redesign the process.

### How

Now the rendering architecture looks like:

* For normal root not-found and certain level of not-found boundary
they're still covered by `LayoutRouter`
* For other error renderings including not-found
* Fully remove the top level not-found boundary, when it renders with
404 error it goes to render the fallback page
* During rendering the fallback page it will check if it should just
renders a 404 error page or render nothing and let the error from inline
flight data to trigger the error boundary

pseudo code
```
try {
  render AppRouter > PageComponent
} catch (err) {
  create ErrorComponent by determine err
  render AppRouter > ErrorComponent
}
```

In this way if the error is thrown from top-level like the page itself
or even from metadata, we can still catch them and render the proper
error page based on the error type.

The problematic is the HMR: introduces a new development mode meta tag
`<meta name="next-error">` to indicate it's 404 so that we don't do
refresh. This reverts the change brougt in #51637 as it will also has
the duplicated rendering problem for root layout if it's included in the
top level not found boundary.

Also fixes the root layout missing issue:

Fixes #52718
Fixes #52739

---------

Co-authored-by: Shu Ding <g@shud.in>
@karlhorky
Copy link
Contributor Author

Thanks for #52790 @huozhi and @ijjk 🙌

However, I noticed that PR was reverted in #52977 before the release of next@13.4.11.

Should we reopen this issue until it's landed permanently?

@ijjk ijjk reopened this Jul 21, 2023
@LagSlug
Copy link

LagSlug commented Jul 21, 2023

I'm not sure if this is related, but the not-found.js file is being resolved to a parent route when set under a dynamic route.

E.g.

In the following tree, under the route /bovines/[id], the not-found.tsx under the [id] directory will be called, but the returned node is not rendered - instead the nearest parent directory with a not-found.tsx is rendered.

.
└── app
    ├── bovines
    │   ├── [id]
    │   │   ├── not-found.tsx  (not rendered when throwing the NotFound error at this route)
    │   │   └── page.tsx
    │   ├── not-found.tsx (this is rendered instead)
    │   └── page.tsx
    ├── favicon.ico
    ├── globals.css
    ├── layout.tsx
    └── page.tsx

ijjk pushed a commit that referenced this issue Jul 21, 2023
Reland #52790
Reverts #52977

was failed due to failed job
[vercel/next.js/actions/runs/5616458194/job/15220295829](https://github.com/vercel/next.js/actions/runs/5616458194/job/15220295829)

Should be fine to resolve with
#52979 now

Fixes #52718
Fixes #52739

---------

Co-authored-by: Alex Kirszenberg <alex.kirszenberg@vercel.com>
Co-authored-by: Tobias Koppers <tobias.koppers@googlemail.com>
@karlhorky
Copy link
Contributor Author

karlhorky commented Jul 24, 2023

Thanks for the re-land in #52985 @huozhi @ijjk 🙌

I can confirm that it's working - the layout is back for not-found now:

CodeSandbox: https://codesandbox.io/p/sandbox/determined-bash-hp979v?file=%2Fpackage.json%3A10%2C24

Kapture.2023-07-24.at.11.13.29.mp4

@github-actions
Copy link
Contributor

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 Aug 16, 2023
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. linear: next Confirmed issue that is tracked by the Next.js team. locked
Projects
None yet
4 participants