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

Extend not-found.js to catch all unmatched routes #47328

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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) {
shuding marked this conversation as resolved.
Show resolved Hide resolved
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"')
})
}
})
}
)