From e20c8c824c9dc8cfde3d4b6eb6c3c7bd2fd2c081 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Thu, 17 Aug 2023 20:27:27 +0200 Subject: [PATCH] Assign default not-found boundary if custom not-found is not present for root layer only (#54185) Fixes #54174 We should only add default not-found boundary to loader tree components for root layout. It was accidently added for children routes before --- .../build/webpack/loaders/next-app-loader.ts | 23 +++++----- .../[id]/page.js | 12 +++++ .../layout.js | 8 ++++ .../dynamic-layout-without-not-found/page.js | 3 ++ .../not-found/basic/app/dynamic/[id]/page.js | 8 ++-- .../app-dir/not-found/basic/app/not-found.js | 2 +- .../e2e/app-dir/not-found/basic/index.test.ts | 46 +++++++++++++++---- 7 files changed, 76 insertions(+), 26 deletions(-) create mode 100644 test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/[id]/page.js create mode 100644 test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/layout.js create mode 100644 test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/page.js diff --git a/packages/next/src/build/webpack/loaders/next-app-loader.ts b/packages/next/src/build/webpack/loaders/next-app-loader.ts index f31755de2072e..3ba6c72cc6acd 100644 --- a/packages/next/src/build/webpack/loaders/next-app-loader.ts +++ b/packages/next/src/build/webpack/loaders/next-app-loader.ts @@ -53,6 +53,8 @@ const GLOBAL_ERROR_FILE_TYPE = 'global-error' const PAGE_SEGMENT = 'page$' const PARALLEL_CHILDREN_SEGMENT = 'children$' +const defaultNotFoundPath = 'next/dist/client/components/not-found-error' + type DirResolver = (pathToResolve: string) => string type PathResolver = ( pathname: string @@ -311,6 +313,14 @@ async function createTreeCodeFromPath( ([, filePath]) => filePath !== undefined ) + // Add default not found error as root not found if not present + const hasNotFound = definedFilePaths.some( + ([type]) => type === 'not-found' + ) + if (isRootLayer && !hasNotFound) { + definedFilePaths.push(['not-found', defaultNotFoundPath]) + } + if (!rootLayout) { const layoutPath = definedFilePaths.find( ([type]) => type === 'layout' @@ -321,17 +331,6 @@ async function createTreeCodeFromPath( globalError = await resolver( `${path.dirname(layoutPath)}/${GLOBAL_ERROR_FILE_TYPE}` ) - - const hasNotFound = definedFilePaths.some( - ([type]) => type === 'not-found' - ) - // Add default not found error as root not found if not present - if (!hasNotFound) { - const notFoundPath = 'next/dist/client/components/not-found-error' - if (notFoundPath) { - definedFilePaths.push(['not-found', notFoundPath]) - } - } } } @@ -350,7 +349,7 @@ async function createTreeCodeFromPath( if (isNotFoundRoute && normalizedParallelKey === 'children') { const notFoundPath = definedFilePaths.find(([type]) => type === 'not-found')?.[1] ?? - 'next/dist/client/components/not-found-error' + defaultNotFoundPath subtreeCode = `{ children: ['__PAGE__', {}, { page: [ diff --git a/test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/[id]/page.js b/test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/[id]/page.js new file mode 100644 index 0000000000000..9618e36735897 --- /dev/null +++ b/test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/[id]/page.js @@ -0,0 +1,12 @@ +import { notFound } from 'next/navigation' + +// avoid static generation to fill the dynamic params +export const dynamic = 'force-dynamic' + +export default function Page({ params: { id } }) { + if (id === '404') { + notFound() + } + + return

{`dynamic-layout-without-not-found [id]`}

+} diff --git a/test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/layout.js b/test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/layout.js new file mode 100644 index 0000000000000..7a2caa5a830c8 --- /dev/null +++ b/test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/layout.js @@ -0,0 +1,8 @@ +export default function Layout({ children }) { + return ( +
+

Dynamic with Layout

+ {children} +
+ ) +} diff --git a/test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/page.js b/test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/page.js new file mode 100644 index 0000000000000..b68eb6e9aebfa --- /dev/null +++ b/test/e2e/app-dir/not-found/basic/app/dynamic-layout-without-not-found/page.js @@ -0,0 +1,3 @@ +export default function Page() { + return
dynamic-with-layout
+} diff --git a/test/e2e/app-dir/not-found/basic/app/dynamic/[id]/page.js b/test/e2e/app-dir/not-found/basic/app/dynamic/[id]/page.js index b900383c1bcbc..bc4249f9d5967 100644 --- a/test/e2e/app-dir/not-found/basic/app/dynamic/[id]/page.js +++ b/test/e2e/app-dir/not-found/basic/app/dynamic/[id]/page.js @@ -3,8 +3,10 @@ import { notFound } from 'next/navigation' // avoid static generation to fill the dynamic params export const dynamic = 'force-dynamic' -export default function Page() { - notFound() +export default function Page({ params: { id } }) { + if (id === '404') { + notFound() + } - return <>{`dynamic [id]`} + return

{`dynamic [id]`}

} diff --git a/test/e2e/app-dir/not-found/basic/app/not-found.js b/test/e2e/app-dir/not-found/basic/app/not-found.js index 9cefbbcf6c820..6de39c97e7e2d 100644 --- a/test/e2e/app-dir/not-found/basic/app/not-found.js +++ b/test/e2e/app-dir/not-found/basic/app/not-found.js @@ -1,7 +1,7 @@ export default function NotFound() { return ( <> -

This Is The Not Found Page

+

Root Not Found

{Date.now()}
diff --git a/test/e2e/app-dir/not-found/basic/index.test.ts b/test/e2e/app-dir/not-found/basic/index.test.ts index e69e8dc982101..e0ee2ff292dd3 100644 --- a/test/e2e/app-dir/not-found/basic/index.test.ts +++ b/test/e2e/app-dir/not-found/basic/index.test.ts @@ -29,7 +29,7 @@ createNextDescribe( it('should use root not-found content for 404 html', async () => { // static /404 page will use /_not-found content const page404 = await next.readFile('.next/server/pages/404.html') - expect(page404).toContain('This Is The Not Found Page') + expect(page404).toContain('Root Not Found') }) } @@ -37,7 +37,7 @@ createNextDescribe( it('should use the not-found page for non-matching routes', async () => { const browser = await next.browser('/random-content') expect(await browser.elementByCss('h1').text()).toContain( - 'This Is The Not Found Page' + 'Root Not Found' ) // should contain root layout content expect(await browser.elementByCss('#layout-nav').text()).toBe('Navbar') @@ -48,11 +48,40 @@ createNextDescribe( const browserDynamic = await next.browser('/dynamic') expect(await browserDynamic.elementByCss('main').text()).toBe('dynamic') - // `/dynamic/[id]` calling notFound() will match the same level not-found boundary - const browserDynamicId = await next.browser('/dynamic/123') - expect(await browserDynamicId.elementByCss('#not-found').text()).toBe( + // `/dynamic/404` calling notFound() will match the same level not-found boundary + const browserDynamic404 = await next.browser('/dynamic/404') + expect(await browserDynamic404.elementByCss('#not-found').text()).toBe( 'dynamic/[id] not found' ) + + const browserDynamicId = await next.browser('/dynamic/123') + expect(await browserDynamicId.elementByCss('#page').text()).toBe( + 'dynamic [id]' + ) + }) + + it('should escalate notFound to parent layout if no not-found boundary present in current layer', async () => { + const browserDynamic = await next.browser( + '/dynamic-layout-without-not-found' + ) + expect(await browserDynamic.elementByCss('h1').text()).toBe( + 'Dynamic with Layout' + ) + + // no not-found boundary in /dynamic-layout-without-not-found, escalate to parent layout to render root not-found + const browserDynamicId = await next.browser( + '/dynamic-layout-without-not-found/404' + ) + expect(await browserDynamicId.elementByCss('h1').text()).toBe( + 'Root Not Found' + ) + + const browserDynamic404 = await next.browser( + '/dynamic-layout-without-not-found/123' + ) + expect(await browserDynamic404.elementByCss('#page').text()).toBe( + 'dynamic-layout-without-not-found [id]' + ) }) if (isNextDev) { @@ -74,10 +103,7 @@ createNextDescribe( const browser = await next.browser('/') await check(() => browser.elementByCss('h1').text(), 'My page') await next.renameFile('./app/page.js', './app/foo.js') - await check( - () => browser.elementByCss('h1').text(), - 'This Is The Not Found Page' - ) + await check(() => browser.elementByCss('h1').text(), 'Root Not Found') await next.renameFile('./app/foo.js', './app/page.js') await check(() => browser.elementByCss('h1').text(), 'My page') }) @@ -86,7 +112,7 @@ createNextDescribe( if (!isNextDev && !isEdge) { 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(html).toContain('Root Not Found') expect( await next.readFile('.next/server/pages-manifest.json') ).toContain('"pages/404.html"')