Consistently use /_not-found for not found page in App Router (#62679)

## What? #62528 caused test/e2e/app-dir/not-found/conflict-route to fail compilation in Turbopack, this compiler error was previously already reported by Turbopack but Next.js didn't show it, which #62528 resolved. This PR changes the handling for the not-found handling to be consistent between development and build, which ensures that the "special" page no longer conflicts with app/not-found/page.js. Closes NEXT-2617 Note: this is a reworked iteration of #62585 which wasn't sufficient.
vercel · Feb 29, 2024 · c262e61 · c262e61
1 parent 9ae437f
commit c262e61
Show file tree

Hide file tree

Showing 24 changed files with 138 additions and 173 deletions.
diff --git a/packages/next-swc/crates/next-core/src/app_structure.rs b/packages/next-swc/crates/next-core/src/app_structure.rs
@@ -1035,54 +1035,38 @@ async fn directory_tree_to_entrypoints_internal_untraced(
 
         // Next.js has this logic in "collect-app-paths", where the root not-found page
         // is considered as its own entry point.
-        let not_found_tree = if components.not_found.is_some() {
-            LoaderTree {
+        let not_found_tree = LoaderTree {
                 page: app_page.clone(),
                 segment: directory_name.clone(),
                 parallel_routes: indexmap! {
                     "children".to_string() => LoaderTree {
                         page: app_page.clone(),
-                        segment: "__DEFAULT__".to_string(),
-                        parallel_routes: IndexMap::new(),
-                        components: Components {
-                            default: Some(get_next_package(app_dir).join("dist/client/components/parallel-route-default.js".to_string())),
-                            ..Default::default()
-                        }.cell(),
-                        global_metadata,
-                    }.cell(),
-                },
-                components: components.without_leafs().cell(),
-                global_metadata,
-            }.cell()
-        } else {
-            // Create default not-found page for production if there's no customized
-            // not-found
-            LoaderTree {
-                page: app_page.clone(),
-                segment: directory_name.to_string(),
-                parallel_routes: indexmap! {
-                    "children".to_string() => LoaderTree {
-                        page: app_page.clone(),
-                        segment: "__PAGE__".to_string(),
-                        parallel_routes: IndexMap::new(),
-                        components: Components {
-                            page: Some(get_next_package(app_dir).join("dist/client/components/not-found-error.js".to_string())),
-                            ..Default::default()
-                        }.cell(),
+                        segment: "/_not-found".to_string(),
+                        parallel_routes: indexmap! {
+                            "children".to_string() => LoaderTree {
+                                page: app_page.clone(),
+                                segment: "__PAGE__".to_string(),
+                                parallel_routes: IndexMap::new(),
+                                components: Components {
+                                    page: components.not_found.or_else(|| Some(get_next_package(app_dir).join("dist/client/components/not-found-error.js".to_string()))),
+                                    ..Default::default()
+                                }.cell(),
+                                global_metadata
+                            }.cell()
+                        },
+                        components: Components::default().cell(),
                         global_metadata,
                     }.cell(),
                 },
                 components: components.without_leafs().cell(),
                 global_metadata,
-            }.cell()
-        };
+            }
+            .cell();
 
         {
-            let app_page = app_page.clone_push_str("not-found")?;
-            add_app_page(app_dir, &mut result, app_page, not_found_tree).await?;
-        }
-        {
-            let app_page = app_page.clone_push_str("_not-found")?;
+            let app_page = app_page
+                .clone_push_str("_not-found")?
+                .complete(PageType::Page)?;
             add_app_page(app_dir, &mut result, app_page, not_found_tree).await?;
         }
     }

diff --git a/packages/next/src/build/entries.ts b/packages/next/src/build/entries.ts
@@ -25,7 +25,11 @@ import {
 } from '../lib/constants'
 import { isAPIRoute } from '../lib/is-api-route'
 import { isEdgeRuntime } from '../lib/is-edge-runtime'
-import { APP_CLIENT_INTERNALS, RSC_MODULE_TYPES } from '../shared/lib/constants'
+import {
+  APP_CLIENT_INTERNALS,
+  RSC_MODULE_TYPES,
+  UNDERSCORE_NOT_FOUND_ROUTE_ENTRY,
+} from '../shared/lib/constants'
 import {
   CLIENT_STATIC_FILES_RUNTIME_AMP,
   CLIENT_STATIC_FILES_RUNTIME_MAIN,
@@ -243,7 +247,9 @@ export function createPagesMapping({
       let pageKey = getPageFromPath(pagePath, pageExtensions)
       if (isAppRoute) {
         pageKey = pageKey.replace(/%5F/g, '_')
-        pageKey = pageKey.replace(/^\/not-found$/g, '/_not-found')
+        if (pageKey === '/not-found') {
+          pageKey = UNDERSCORE_NOT_FOUND_ROUTE_ENTRY
+        }
       }
 
       const normalizedPath = normalizePathSep(
@@ -277,7 +283,8 @@ export function createPagesMapping({
         // If there's any app pages existed, add a default not-found page.
         // If there's any custom not-found page existed, it will override the default one.
         ...(hasAppPages && {
-          '/_not-found': 'next/dist/client/components/not-found-error',
+          [UNDERSCORE_NOT_FOUND_ROUTE_ENTRY]:
+            'next/dist/client/components/not-found-error',
         }),
         ...pages,
       }
@@ -582,6 +589,7 @@ export async function createEntrypoints(
           : pagesType === PAGE_TYPES.APP
           ? posix.join('app', bundleFile)
           : bundleFile.slice(1)
+
       const absolutePagePath = mappings[page]
 
       // Handle paths that have aliases

diff --git a/packages/next/src/build/index.ts b/packages/next/src/build/index.ts
@@ -72,6 +72,8 @@ import {
   MIDDLEWARE_REACT_LOADABLE_MANIFEST,
   SERVER_REFERENCE_MANIFEST,
   FUNCTIONS_CONFIG_MANIFEST,
+  UNDERSCORE_NOT_FOUND_ROUTE_ENTRY,
+  UNDERSCORE_NOT_FOUND_ROUTE,
 } from '../shared/lib/constants'
 import { getSortedRoutes, isDynamicRoute } from '../shared/lib/router/utils'
 import type { __ApiPreviewProps } from '../server/api-utils'
@@ -1034,7 +1036,7 @@ export default async function build(
 
       const conflictingPublicFiles: string[] = []
       const hasPages404 = mappedPages['/404']?.startsWith(PAGES_DIR_ALIAS)
-      const hasApp404 = !!mappedAppPages?.['/_not-found']
+      const hasApp404 = !!mappedAppPages?.[UNDERSCORE_NOT_FOUND_ROUTE_ENTRY]
       const hasCustomErrorPage =
         mappedPages['/_error'].startsWith(PAGES_DIR_ALIAS)
 
@@ -2436,7 +2438,9 @@ export default async function build(
         !hasPages500 && !hasNonStaticErrorPage && !customAppGetInitialProps
 
       const combinedPages = [...staticPages, ...ssgPages]
-      const isApp404Static = appStaticPaths.has('/_not-found')
+      const isApp404Static = appStaticPaths.has(
+        UNDERSCORE_NOT_FOUND_ROUTE_ENTRY
+      )
       const hasStaticApp404 = hasApp404 && isApp404Static
 
       // we need to trigger automatic exporting when we have
@@ -2679,7 +2683,7 @@ export default async function build(
 
             routes.forEach((route) => {
               if (isDynamicRoute(page) && route === page) return
-              if (route === '/_not-found') return
+              if (route === UNDERSCORE_NOT_FOUND_ROUTE) return
 
               const {
                 revalidate = appConfig.revalidate ?? false,

diff --git a/packages/next/src/build/utils.ts b/packages/next/src/build/utils.ts
@@ -52,7 +52,10 @@ import {
   INSTRUMENTATION_HOOK_FILENAME,
   WEBPACK_LAYERS,
 } from '../lib/constants'
-import { MODERN_BROWSERSLIST_TARGET } from '../shared/lib/constants'
+import {
+  MODERN_BROWSERSLIST_TARGET,
+  UNDERSCORE_NOT_FOUND_ROUTE,
+} from '../shared/lib/constants'
 import prettyBytes from '../lib/pretty-bytes'
 import { getRouteRegex } from '../shared/lib/router/utils/route-regex'
 import { getRouteMatcher } from '../shared/lib/router/utils/route-matcher'
@@ -688,7 +691,10 @@ export async function printTreeView(
   })
 
   // If there's no app /_notFound page present, then the 404 is still using the pages/404
-  if (!lists.pages.includes('/404') && !lists.app?.includes('/_not-found')) {
+  if (
+    !lists.pages.includes('/404') &&
+    !lists.app?.includes(UNDERSCORE_NOT_FOUND_ROUTE)
+  ) {
     lists.pages = [...lists.pages, '/404']
   }
 

diff --git a/packages/next/src/build/webpack/loaders/next-app-loader.ts b/packages/next/src/build/webpack/loaders/next-app-loader.ts
@@ -1,5 +1,9 @@
 import type webpack from 'next/dist/compiled/webpack/webpack'
-import type { ValueOf } from '../../../shared/lib/constants'
+import {
+  UNDERSCORE_NOT_FOUND_ROUTE,
+  UNDERSCORE_NOT_FOUND_ROUTE_ENTRY,
+  type ValueOf,
+} from '../../../shared/lib/constants'
 import type { ModuleReference, CollectedMetadata } from './metadata/types'
 
 import path from 'path'
@@ -192,7 +196,8 @@ async function createTreeCodeFromPath(
   globalError: string
 }> {
   const splittedPath = pagePath.split(/[\\/]/, 1)
-  const isNotFoundRoute = page === '/_not-found'
+  const isNotFoundRoute = page === UNDERSCORE_NOT_FOUND_ROUTE_ENTRY
+
   const isDefaultNotFound = isAppBuiltinNotFoundPage(pagePath)
   const appDirPrefix = isDefaultNotFound ? APP_DIR_ALIAS : splittedPath[0]
   const hasRootNotFound = await resolver(
@@ -410,14 +415,16 @@ async function createTreeCodeFromPath(
           defaultNotFoundPath
         nestedCollectedAsyncImports.push(notFoundPath)
         subtreeCode = `{
-          children: ['${PAGE_SEGMENT_KEY}', {}, {
-            page: [
-              () => import(/* webpackMode: "eager" */ ${JSON.stringify(
-                notFoundPath
-              )}),
-              ${JSON.stringify(notFoundPath)}
-            ]
-          }]
+          children: [${JSON.stringify(UNDERSCORE_NOT_FOUND_ROUTE)}, {
+            children: ['${PAGE_SEGMENT_KEY}', {}, {
+              page: [
+                () => import(/* webpackMode: "eager" */ ${JSON.stringify(
+                  notFoundPath
+                )}),
+                ${JSON.stringify(notFoundPath)}
+              ]
+            }]
+          }, {}]
         }`
       }
 

diff --git a/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts b/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts
@@ -22,6 +22,7 @@ import {
   DEFAULT_RUNTIME_WEBPACK,
   EDGE_RUNTIME_WEBPACK,
   SERVER_REFERENCE_MANIFEST,
+  UNDERSCORE_NOT_FOUND_ROUTE_ENTRY,
 } from '../../../shared/lib/constants'
 import {
   getActions,
@@ -333,6 +334,23 @@ export class FlightClientEntryPlugin {
           bundlePath,
           absolutePagePath: entryRequest,
         })
+
+        // The webpack implementation of writing the client reference manifest relies on all entrypoints writing a page.js even when there is no client components in the page.
+        // It needs the file in order to write the reference manifest for the path in the `.next/server` folder.
+        // TODO-APP: This could be better handled, however Turbopack does not have the same problem as we resolve client components in a single graph.
+        if (
+          name === `app${UNDERSCORE_NOT_FOUND_ROUTE_ENTRY}` &&
+          bundlePath === 'app/not-found'
+        ) {
+          clientEntriesToInject.push({
+            compiler,
+            compilation,
+            entryName: name,
+            clientComponentImports: {},
+            bundlePath: `app${UNDERSCORE_NOT_FOUND_ROUTE_ENTRY}`,
+            absolutePagePath: entryRequest,
+          })
+        }
       }
 
       // Make sure CSS imports are deduplicated before injecting the client entry

diff --git a/packages/next/src/build/webpack/plugins/flight-manifest-plugin.ts b/packages/next/src/build/webpack/plugins/flight-manifest-plugin.ts
@@ -456,13 +456,6 @@ export class ClientReferenceManifestPlugin {
         manifestEntryFiles.push(entryName.replace(/\/page(\.[^/]+)?$/, '/page'))
       }
 
-      // Special case for the root not-found page.
-      // dev: app/not-found
-      // prod: app/_not-found
-      if (/^app\/_?not-found(\.[^.]+)?$/.test(entryName)) {
-        manifestEntryFiles.push(this.dev ? 'app/not-found' : 'app/_not-found')
-      }
-
       const groupName = entryNameToGroupName(entryName)
       if (!manifestsPerGroup.has(groupName)) {
         manifestsPerGroup.set(groupName, [])
@@ -503,16 +496,6 @@ export class ClientReferenceManifestPlugin {
           pagePath.slice('app'.length)
         )}]=${json}`
       ) as unknown as webpack.sources.RawSource
-
-      if (pagePath === 'app/not-found') {
-        // Create a separate special manifest for the root not-found page.
-        assets['server/app/_not-found_' + CLIENT_REFERENCE_MANIFEST + '.js'] =
-          new sources.RawSource(
-            `globalThis.__RSC_MANIFEST=(globalThis.__RSC_MANIFEST||{});globalThis.__RSC_MANIFEST[${JSON.stringify(
-              '/_not-found'
-            )}]=${json}`
-          ) as unknown as webpack.sources.RawSource
-      }
     }
 
     pluginState.ASYNC_CLIENT_MODULES = []

diff --git a/packages/next/src/build/webpack/plugins/next-trace-entrypoints-plugin.ts b/packages/next/src/build/webpack/plugins/next-trace-entrypoints-plugin.ts
@@ -7,6 +7,7 @@ import type { NodeFileTraceReasons } from 'next/dist/compiled/@vercel/nft'
 import {
   CLIENT_REFERENCE_MANIFEST,
   TRACE_OUTPUT_VERSION,
+  UNDERSCORE_NOT_FOUND_ROUTE_ENTRY,
 } from '../../../shared/lib/constants'
 import { webpack, sources } from 'next/dist/compiled/webpack/webpack'
 import {
@@ -242,8 +243,7 @@ export class TraceEntryPointsPlugin implements webpack.WebpackPluginInstance {
           // include the client reference manifest
           const clientManifestsForPage =
             entrypoint.name.endsWith('/page') ||
-            entrypoint.name === 'app/not-found' ||
-            entrypoint.name === 'app/_not-found'
+            entrypoint.name === UNDERSCORE_NOT_FOUND_ROUTE_ENTRY
               ? nodePath.join(
                   outputPath,
                   '..',

diff --git a/packages/next/src/client/components/router-reducer/is-navigating-to-new-root-layout.ts b/packages/next/src/client/components/router-reducer/is-navigating-to-new-root-layout.ts
@@ -1,5 +1,4 @@
 import type { FlightRouterState } from '../../../server/app-render/types'
-import { GLOBAL_NOT_FOUND_SEGMENT_KEY } from '../../../shared/lib/segment'
 
 export function isNavigatingToNewRootLayout(
   currentTree: FlightRouterState,
@@ -9,9 +8,6 @@ export function isNavigatingToNewRootLayout(
   const currentTreeSegment = currentTree[0]
   const nextTreeSegment = nextTree[0]
 
-  // We currently special-case the global not found segment key, but we don't want it to be treated as a root layout change
-  if (currentTreeSegment === GLOBAL_NOT_FOUND_SEGMENT_KEY) return false
-
   // If any segment is different before we find the root layout, the root layout has changed.
   // E.g. /same/(group1)/layout.js -> /same/(group2)/layout.js
   // First segment is 'same' for both, keep looking. (group1) changed to (group2) before the root layout was found, it must have changed.

diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts
@@ -22,10 +22,7 @@ import { handleMutable } from '../handle-mutable'
 import { applyFlightData } from '../apply-flight-data'
 import { prefetchQueue } from './prefetch-reducer'
 import { createEmptyCacheNode } from '../../app-router'
-import {
-  DEFAULT_SEGMENT_KEY,
-  GLOBAL_NOT_FOUND_SEGMENT_KEY,
-} from '../../../../shared/lib/segment'
+import { DEFAULT_SEGMENT_KEY } from '../../../../shared/lib/segment'
 import {
   listenForDynamicRequest,
   updateCacheNodeOnNavigation,
@@ -203,28 +200,6 @@ function navigateReducer_noPPR(
               prefetchEntryCacheStatus === PrefetchCacheEntryStatus.reusable
           )
 
-          if (
-            !applied &&
-            // if we've navigated away from the global not found segment but didn't apply the flight data, we need to refetch
-            // as otherwise we'd be incorrectly using the global not found cache node for the incoming page
-            currentTree[0] === GLOBAL_NOT_FOUND_SEGMENT_KEY
-          ) {
-            applied = addRefetchToLeafSegments(
-              cache,
-              currentCache,
-              flightSegmentPath,
-              treePatch,
-              // eslint-disable-next-line no-loop-func
-              () =>
-                fetchServerResponse(
-                  url,
-                  currentTree,
-                  state.nextUrl,
-                  state.buildId
-                )
-            )
-          }
-
           const hardNavigate = shouldHardNavigate(
             // TODO-APP: remove ''
             flightSegmentPathWithLeadingEmpty,
@@ -450,28 +425,6 @@ function navigateReducer_PPR(
                 prefetchEntryCacheStatus === PrefetchCacheEntryStatus.reusable
             )
 
-            if (
-              !applied &&
-              // if we've navigated away from the global not found segment but didn't apply the flight data, we need to refetch
-              // as otherwise we'd be incorrectly using the global not found cache node for the incoming page
-              currentTree[0] === GLOBAL_NOT_FOUND_SEGMENT_KEY
-            ) {
-              applied = addRefetchToLeafSegments(
-                cache,
-                currentCache,
-                flightSegmentPath,
-                treePatch,
-                // eslint-disable-next-line no-loop-func
-                () =>
-                  fetchServerResponse(
-                    url,
-                    currentTree,
-                    state.nextUrl,
-                    state.buildId
-                  )
-              )
-            }
-
             const hardNavigate = shouldHardNavigate(
               // TODO-APP: remove ''
               flightSegmentPathWithLeadingEmpty,

diff --git a/packages/next/src/export/routes/app-page.ts b/packages/next/src/export/routes/app-page.ts
@@ -41,6 +41,7 @@ export async function exportAppPage(
   fileWriter: FileWriter
 ): Promise<ExportRouteResult> {
   // If the page is `/_not-found`, then we should update the page to be `/404`.
+  // UNDERSCORE_NOT_FOUND_ROUTE value used here, however we don't want to import it here as it causes constants to be inlined which we don't want here.
   if (page === '/_not-found') {
     pathname = '/404'
   }