Skip to content

Commit

Permalink
Extend not-found.js to catch all unmatched routes (#47328)
Browse files Browse the repository at this point in the history
This PR continues the work of #45867, that treats the root-level `not-found.js` file inside app dir as the global 404 page, if it exists. Previously, it fallbacks to the /404 route inside pages (and the default one if `404.js` isn't specified).

In the implementation, we include `/not-found` in `appPaths` during the build, and treat it as the special `/404` path during pre-rendering. In the renderer, if the `/404` pathname is being handled, we always render the not found boundary.

And finally inside the server, we check if `/not-found` exists before picking up the `/404` component.

A deployed example: https://not-found-shuding1.vercel.app/balasdkjfaklsdf

fix NEXT-463 ([link](https://linear.app/vercel/issue/NEXT-463))
  • Loading branch information
shuding authored Mar 22, 2023
1 parent de92781 commit 783d7d5
Show file tree
Hide file tree
Showing 12 changed files with 168 additions and 23 deletions.
50 changes: 45 additions & 5 deletions packages/next/src/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,17 @@ export default async function build(
appPaths = await nextBuildSpan
.traceChild('collect-app-paths')
.traceAsyncFn(() =>
recursiveReadDir(appDir, validFileMatcher.isAppRouterPage)
recursiveReadDir(appDir, (absolutePath) => {
if (validFileMatcher.isAppRouterPage(absolutePath)) {
return true
}
// For now we only collect the root /not-found page in the app
// directory as the 404 fallback.
if (validFileMatcher.isRootNotFound(absolutePath)) {
return true
}
return false
})
)
}

Expand Down Expand Up @@ -653,6 +663,7 @@ export default async function build(

const conflictingPublicFiles: string[] = []
const hasPages404 = mappedPages['/404']?.startsWith(PAGES_DIR_ALIAS)
const hasApp404 = !!mappedAppPages?.['/not-found']
const hasCustomErrorPage =
mappedPages['/_error'].startsWith(PAGES_DIR_ALIAS)

Expand Down Expand Up @@ -2159,7 +2170,8 @@ export default async function build(
// Since custom _app.js can wrap the 404 page we have to opt-out of static optimization if it has getInitialProps
// Only export the static 404 when there is no /_error present
const useStatic404 =
!customAppGetInitialProps && (!hasNonStaticErrorPage || hasPages404)
!customAppGetInitialProps &&
(!hasNonStaticErrorPage || hasPages404 || hasApp404)

if (invalidPages.size > 0) {
const err = new Error(
Expand Down Expand Up @@ -2463,6 +2475,7 @@ export default async function build(

routes.forEach((route) => {
if (isDynamicRoute(page) && route === page) return
if (route === '/not-found') return

let revalidate = exportConfig.initialPageRevalidationMap[route]

Expand Down Expand Up @@ -2664,9 +2677,36 @@ export default async function build(
})
}

// Only move /404 to /404 when there is no custom 404 as in that case we don't know about the 404 page
if (!hasPages404 && useStatic404) {
await moveExportedPage('/_error', '/404', '/404', false, 'html')
async function moveExportedAppNotFoundTo404() {
return staticGenerationSpan
.traceChild('move-exported-app-not-found-')
.traceAsyncFn(async () => {
const orig = path.join(
distDir,
'server',
'app',
'not-found.html'
)
const updatedRelativeDest = path
.join('pages', '404.html')
.replace(/\\/g, '/')
await promises.copyFile(
orig,
path.join(distDir, 'server', updatedRelativeDest)
)
pagesManifest['/404'] = updatedRelativeDest
})
}

// If there's /not-found inside app, we prefer it over the pages 404
if (hasApp404 && useStatic404) {
// await moveExportedPage('/_error', '/404', '/404', false, 'html')
await moveExportedAppNotFoundTo404()
} else {
// Only move /404 to /404 when there is no custom 404 as in that case we don't know about the 404 page
if (!hasPages404 && useStatic404) {
await moveExportedPage('/_error', '/404', '/404', false, 'html')
}
}

if (useDefaultStatic500) {
Expand Down
11 changes: 6 additions & 5 deletions packages/next/src/build/webpack/loaders/next-app-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,13 @@ async function createTreeCodeFromPath(
})
)

const definedFilePaths = filePaths.filter(
([, filePath]) => filePath !== undefined
)

if (!rootLayout) {
const layoutPath = filePaths.find(
([type, filePath]) => type === 'layout' && !!filePath
const layoutPath = definedFilePaths.find(
([type]) => type === 'layout'
)?.[1]
rootLayout = layoutPath

Expand All @@ -232,9 +236,6 @@ async function createTreeCodeFromPath(
}
}

const definedFilePaths = filePaths.filter(
([, filePath]) => filePath !== undefined
)
props[parallelKey] = `[
'${
Array.isArray(parallelSegment) ? parallelSegment[0] : parallelSegment
Expand Down
3 changes: 2 additions & 1 deletion packages/next/src/export/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -465,10 +465,11 @@ export default async function exportPage({
try {
curRenderOpts.params ||= {}

const isNotFoundPage = page === '/not-found'
const result = await renderToHTMLOrFlight(
req as any,
res as any,
page,
isNotFoundPage ? '/404' : page,
query,
curRenderOpts as any
)
Expand Down
28 changes: 27 additions & 1 deletion packages/next/src/server/app-render/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,7 @@ export async function renderToHTMLOrFlight(
rootLayoutIncluded: rootLayoutIncludedAtThisLevelOrAbove,
injectedCSS: injectedCSSWithCurrentLayout,
injectedFontPreloadTags: injectedFontPreloadTagsWithCurrentLayout,
asNotFound,
})

const childProp: ChildProp = {
Expand Down Expand Up @@ -739,8 +740,24 @@ export async function renderToHTMLOrFlight(

const isClientComponent = isClientReference(layoutOrPageMod)

// If it's a not found route, and we don't have any matched parallel
// routes, we try to render the not found component if it exists.
let notFoundComponent = {}
if (asNotFound && !parallelRouteMap.length && NotFound) {
notFoundComponent = {
children: (
<>
<meta name="robots" content="noindex" />
{notFoundStyles}
<NotFound />
</>
),
}
}

const props = {
...parallelRouteComponents,
...notFoundComponent,
// TODO-APP: params and query have to be blocked parallel route names. Might have to add a reserved name list.
// Params are always the current params that apply to the layout
// If you have a `/dashboard/[team]/layout.js` it will provide `team` as a param but not anything further down.
Expand Down Expand Up @@ -847,6 +864,7 @@ export async function renderToHTMLOrFlight(
injectedCSS,
injectedFontPreloadTags,
rootLayoutIncluded,
asNotFound,
}: {
createSegmentPath: CreateSegmentPath
loaderTreeToFilter: LoaderTree
Expand All @@ -858,6 +876,7 @@ export async function renderToHTMLOrFlight(
injectedCSS: Set<string>
injectedFontPreloadTags: Set<string>
rootLayoutIncluded: boolean
asNotFound?: boolean
}): Promise<FlightDataPath> => {
const [segment, parallelRoutes, components] = loaderTreeToFilter

Expand Down Expand Up @@ -931,6 +950,7 @@ export async function renderToHTMLOrFlight(
injectedFontPreloadTags,
// This is intentionally not "rootLayoutIncludedAtThisLevelOrAbove" as createComponentTree starts at the current level and does a check for "rootLayoutAtThisLevel" too.
rootLayoutIncluded: rootLayoutIncluded,
asNotFound,
}
)

Expand Down Expand Up @@ -988,6 +1008,7 @@ export async function renderToHTMLOrFlight(
injectedCSS: injectedCSSWithCurrentLayout,
injectedFontPreloadTags: injectedFontPreloadTagsWithCurrentLayout,
rootLayoutIncluded: rootLayoutIncludedAtThisLevelOrAbove,
asNotFound,
})

if (typeof path[path.length - 1] !== 'string') {
Expand Down Expand Up @@ -1025,6 +1046,7 @@ export async function renderToHTMLOrFlight(
injectedCSS: new Set(),
injectedFontPreloadTags: new Set(),
rootLayoutIncluded: false,
asNotFound: pathname === '/404',
})
).slice(1),
]
Expand Down Expand Up @@ -1410,7 +1432,11 @@ export async function renderToHTMLOrFlight(
}
// End of action request handling.

const renderResult = new RenderResult(await bodyResult({}))
const renderResult = new RenderResult(
await bodyResult({
asNotFound: pathname === '/404',
})
)

if (staticGenerationStore.pendingRevalidates) {
await Promise.all(staticGenerationStore.pendingRevalidates)
Expand Down
34 changes: 24 additions & 10 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2181,16 +2181,30 @@ export default abstract class Server<ServerOptions extends Options = Options> {
let using404Page = false

// use static 404 page if available and is 404 response
if (is404 && (await this.hasPage('/404'))) {
result = await this.findPageComponents({
pathname: '/404',
query,
params: {},
isAppPath: false,
// Ensuring can't be done here because you never "match" a 404 route.
shouldEnsure: true,
})
using404Page = result !== null
if (is404) {
if (this.hasAppDir) {
// Use the not-found entry in app directory
result = await this.findPageComponents({
pathname: '/not-found',
query,
params: {},
isAppPath: true,
shouldEnsure: true,
})
using404Page = result !== null
}

if (!result && (await this.hasPage('/404'))) {
result = await this.findPageComponents({
pathname: '/404',
query,
params: {},
isAppPath: false,
// Ensuring can't be done here because you never "match" a 404 route.
shouldEnsure: true,
})
using404Page = result !== null
}
}
let statusPage = `/${res.statusCode}`

Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/server/lib/app-dir-module.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ComponentsType } from '../../build/webpack/loaders/next-app-loader'
import type { ComponentsType } from '../../build/webpack/loaders/next-app-loader'

/**
* LoaderTree is generated in next-app-loader.
Expand Down
15 changes: 15 additions & 0 deletions packages/next/src/server/lib/find-page-file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ export function createValidFileMatcher(
pageExtensions
)}$`
)
const leafOnlyNotFoundFileRegex = new RegExp(
`^not-found\\.${getExtensionRegexString(pageExtensions)}$`
)
/** TODO-METADATA: support other metadata routes
* regex for:
*
Expand Down Expand Up @@ -125,9 +128,21 @@ export function createValidFileMatcher(
return validExtensionFileRegex.test(filePath) || isMetadataFile(filePath)
}

function isRootNotFound(filePath: string) {
if (!appDirPath) {
return false
}
if (!filePath.startsWith(appDirPath + sep)) {
return false
}
const rest = filePath.slice(appDirPath.length + 1)
return leafOnlyNotFoundFileRegex.test(rest)
}

return {
isPageFile,
isAppRouterPage,
isMetadataFile,
isRootNotFound,
}
}
10 changes: 10 additions & 0 deletions test/e2e/app-dir/not-found/app/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export default function Layout({ children }) {
return (
<html>
<head>
<title>Hello World</title>
</head>
<body>{children}</body>
</html>
)
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/not-found/app/not-found.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <h1>This Is The Not Found Page</h1>
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/not-found/app/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <h1>My page</h1>
}
5 changes: 5 additions & 0 deletions test/e2e/app-dir/not-found/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = {
experimental: {
appDir: true,
},
}
27 changes: 27 additions & 0 deletions test/e2e/app-dir/not-found/not-found.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { createNextDescribe } from 'e2e-utils'

createNextDescribe(
'app dir - not-found',
{
files: __dirname,
skipDeployment: true,
},
({ next, isNextDev }) => {
describe('root not-found page', () => {
it('should use the not-found page for non-matching routes', async () => {
const html = await next.render('/random-content')
expect(html).toContain('This Is The Not Found Page')
})

if (!isNextDev) {
it('should create the 404 mapping and copy the file to pages', async () => {
const html = await next.readFile('.next/server/pages/404.html')
expect(html).toContain('This Is The Not Found Page')
expect(
await next.readFile('.next/server/pages-manifest.json')
).toContain('"pages/404.html"')
})
}
})
}
)

0 comments on commit 783d7d5

Please sign in to comment.