fix: use static 404 for non-prerendered dynamic routes without fallback (#1795)

* fix: rewrite fallback false routes to 404 page

* fix: allow ISR 404 pages to use the ODB handler

* chore: update tests to expect static 404 pages for fallback false

* chore: fix boolean type on is404Isr param

* chore: add tests for 404 redirects

Author: orinokai

cypress/integration/default/dynamic-routes.spec.ts

@@ -49,7 +49,7 @@ describe('Dynamic Routing', () => {
     cy.request({ url: '/getStaticProps/3/', headers: { 'x-nf-debug-logging': '1' }, failOnStatusCode: false }).then(
       (res) => {
         expect(res.status).to.eq(404)
-        expect(res.headers).to.have.property('x-nf-render-mode', 'odb')
+        expect(res.headers).to.not.have.property('x-nf-render-mode')
         expect(res.body).to.contain('Custom 404')
       },
     )
@@ -102,7 +102,7 @@ describe('Dynamic Routing', () => {
       failOnStatusCode: false,
     }).then((res) => {
       expect(res.status).to.eq(404)
-      expect(res.headers).to.have.property('x-nf-render-mode', 'odb')
+      expect(res.headers).to.not.have.property('x-nf-render-mode')
       expect(res.body).to.contain('Custom 404')
     })
   })

packages/runtime/src/helpers/redirects.ts

@@ -19,6 +19,7 @@ import {
   isApiRoute,
   redirectsForNextRoute,
   redirectsForNextRouteWithData,
+  redirectsForNext404Route,
   routeToDataRoute,
 } from './utils'
 
@@ -187,13 +188,15 @@ const generateDynamicRewrites = ({
   basePath,
   buildId,
   i18n,
+  is404Isr,
 }: {
   dynamicRoutes: RoutesManifest['dynamicRoutes']
   prerenderedDynamicRoutes: PrerenderManifest['dynamicRoutes']
   basePath: string
   i18n: NextConfig['i18n']
   buildId: string
   middleware: Array<string>
+  is404Isr: boolean
 }): {
   dynamicRoutesThatMatchMiddleware: Array<string>
   dynamicRewrites: NetlifyConfig['redirects']
@@ -207,9 +210,11 @@ const generateDynamicRewrites = ({
     if (route.page in prerenderedDynamicRoutes) {
       if (matchesMiddleware(middleware, route.page)) {
         dynamicRoutesThatMatchMiddleware.push(route.page)
+      } else if (prerenderedDynamicRoutes[route.page].fallback === false && !is404Isr) {
+        dynamicRewrites.push(...redirectsForNext404Route({ route: route.page, buildId, basePath, i18n }))
       } else {
         dynamicRewrites.push(
-          ...redirectsForNextRoute({ buildId, route: route.page, basePath, to: ODB_FUNCTION_PATH, status: 200, i18n }),
+          ...redirectsForNextRoute({ route: route.page, buildId, basePath, to: ODB_FUNCTION_PATH, i18n }),
         )
       }
     } else {
@@ -263,6 +268,10 @@ export const generateRedirects = async ({
 
   const staticRouteEntries = Object.entries(prerenderedStaticRoutes)
 
+  const is404Isr = staticRouteEntries.some(
+    ([route, { initialRevalidateSeconds }]) => is404Route(route, i18n) && initialRevalidateSeconds !== false,
+  )
+
   const routesThatMatchMiddleware: Array<string> = []
 
   const { staticRoutePaths, staticIsrRewrites, staticIsrRoutesThatMatchMiddleware } = generateStaticIsrRewrites({
@@ -295,6 +304,7 @@ export const generateRedirects = async ({
     basePath,
     buildId,
     i18n,
+    is404Isr,
   })
   netlifyConfig.redirects.push(...dynamicRewrites)
   routesThatMatchMiddleware.push(...dynamicRoutesThatMatchMiddleware)

packages/runtime/src/helpers/utils.ts

@@ -72,9 +72,18 @@ export const netlifyRoutesForNextRouteWithData = ({ route, dataRoute }: { route:
 export const routeToDataRoute = (route: string, buildId: string, locale?: string) =>
   `/_next/data/${buildId}${locale ? `/${locale}` : ''}${route === '/' ? '/index' : route}.json`
 
-const netlifyRoutesForNextRoute = (route: string, buildId: string, i18n?: I18n): Array<string> => {
+const netlifyRoutesForNextRoute = (
+  route: string,
+  buildId: string,
+  i18n?: I18n,
+): Array<{ redirect: string; locale: string | false }> => {
   if (!i18n?.locales?.length) {
-    return netlifyRoutesForNextRouteWithData({ route, dataRoute: routeToDataRoute(route, buildId) })
+    return netlifyRoutesForNextRouteWithData({ route, dataRoute: routeToDataRoute(route, buildId) }).map(
+      (redirect) => ({
+        redirect,
+        locale: false,
+      }),
+    )
   }
   const { locales, defaultLocale } = i18n
   const routes = []
@@ -87,7 +96,10 @@ const netlifyRoutesForNextRoute = (route: string, buildId: string, i18n?: I18n):
       ...netlifyRoutesForNextRouteWithData({
         route: locale === defaultLocale ? route : `/${locale}${route}`,
         dataRoute,
-      }),
+      }).map((redirect) => ({
+        redirect,
+        locale,
+      })),
     )
   })
   return routes
@@ -115,13 +127,33 @@ export const redirectsForNextRoute = ({
   status?: number
   force?: boolean
 }): NetlifyConfig['redirects'] =>
-  netlifyRoutesForNextRoute(route, buildId, i18n).map((redirect) => ({
+  netlifyRoutesForNextRoute(route, buildId, i18n).map(({ redirect }) => ({
     from: `${basePath}${redirect}`,
     to,
     status,
     force,
   }))
 
+export const redirectsForNext404Route = ({
+  route,
+  buildId,
+  basePath,
+  i18n,
+  force = false,
+}: {
+  route: string
+  buildId: string
+  basePath: string
+  i18n: I18n
+  force?: boolean
+}): NetlifyConfig['redirects'] =>
+  netlifyRoutesForNextRoute(route, buildId, i18n).map(({ redirect, locale }) => ({
+    from: `${basePath}${redirect}`,
+    to: locale ? `${basePath}/server/pages/${locale}/404.html` : `${basePath}/server/pages/404.html`,
+    status: 404,
+    force,
+  }))
+
 export const redirectsForNextRouteWithData = ({
   route,
   dataRoute,

test/helpers/utils.spec.ts

@@ -1,30 +1,39 @@
 import Chance from 'chance'
 import { ExperimentalConfig } from 'next/dist/server/config-shared'
-import { getCustomImageResponseHeaders, getRemotePatterns, ImagesConfig } from '../../packages/runtime/src/helpers/utils'
+import {
+  getCustomImageResponseHeaders,
+  getRemotePatterns,
+  ImagesConfig,
+  redirectsForNext404Route,
+} from '../../packages/runtime/src/helpers/utils'
 
 const chance = new Chance()
 
 describe('getCustomImageResponseHeaders', () => {
   it('returns null when no custom image response headers are found', () => {
-    const mockHeaders = [{
-      for: '/test',
-      values: {
-        'X-Foo': chance.string()
-      }
-    }]
+    const mockHeaders = [
+      {
+        for: '/test',
+        values: {
+          'X-Foo': chance.string(),
+        },
+      },
+    ]
 
     expect(getCustomImageResponseHeaders(mockHeaders)).toBe(null)
   })
 
   it('returns header values when custom image response headers are found', () => {
     const mockFooValue = chance.string()
 
-    const mockHeaders = [{
-      for: '/_next/image/',
-      values: {
-        'X-Foo': mockFooValue
-      }
-    }]
+    const mockHeaders = [
+      {
+        for: '/_next/image/',
+        values: {
+          'X-Foo': mockFooValue,
+        },
+      },
+    ]
 
     const result = getCustomImageResponseHeaders(mockHeaders)
     expect(result).toStrictEqual({
@@ -38,31 +47,23 @@ describe('getRemotePatterns', () => {
   let mockImages
   beforeEach(() => {
     mockExperimentalConfig = {
-      images: {}
+      images: {},
     } as ExperimentalConfig
-  
+
     mockImages = {
-      deviceSizes: [
-        640,  750,  828,
-        1080, 1200, 1920,
-        2048, 3840
-      ],
-      imageSizes: [
-        16,  32,  48,  64,
-        96, 128, 256, 384
-      ],
+      deviceSizes: [640, 750, 828, 1080, 1200, 1920, 2048, 3840],
+      imageSizes: [16, 32, 48, 64, 96, 128, 256, 384],
       path: '/_next/image',
       loader: 'default',
       domains: [],
       disableStaticImages: false,
       minimumCacheTTL: 60,
-      formats: [ 'image/avif', 'image/webp' ],
+      formats: ['image/avif', 'image/webp'],
       dangerouslyAllowSVG: false,
       contentSecurityPolicy: "script-src 'none'; frame-src 'none'; sandbox;",
       unoptimized: false,
-      remotePatterns: []
+      remotePatterns: [],
     } as ImagesConfig
-  
   })
 
   it('returns the remote patterns found under experimental.images', () => {
@@ -73,7 +74,7 @@ describe('getRemotePatterns', () => {
       },
     ]
     const result = getRemotePatterns(mockExperimentalConfig, mockImages)
-  
+
     expect(result).toStrictEqual(mockExperimentalConfig.images?.remotePatterns)
   })
 
@@ -85,12 +86,49 @@ describe('getRemotePatterns', () => {
       },
     ]
     const result = getRemotePatterns(mockExperimentalConfig, mockImages)
-  
-    expect(result).toStrictEqual(mockImages.remotePatterns)  
+
+    expect(result).toStrictEqual(mockImages.remotePatterns)
   })
-  
+
   it('returns an empty array', () => {
     const result = getRemotePatterns(mockExperimentalConfig, mockImages)
     expect(result).toStrictEqual([])
   })
 })
+
+describe('redirectsForNext404Route', () => {
+  it('returns static 404 redirects', () => {
+    const mockRoute = {
+      route: '/test',
+      buildId: 'test',
+      basePath: '',
+      i18n: null,
+    }
+
+    expect(redirectsForNext404Route(mockRoute)).toStrictEqual([
+      { force: false, from: '/_next/data/test/test.json', status: 404, to: '/server/pages/404.html' },
+      { force: false, from: '/test', status: 404, to: '/server/pages/404.html' },
+    ])
+  })
+
+  it('returns localised static 404 redirects when i18n locales are provided', () => {
+    const mockRoute = {
+      route: '/test',
+      buildId: 'test',
+      basePath: '',
+      i18n: {
+        defaultLocale: 'en',
+        locales: ['en', 'es', 'fr'],
+      },
+    }
+
+    expect(redirectsForNext404Route(mockRoute)).toStrictEqual([
+      { force: false, from: '/_next/data/test/en/test.json', status: 404, to: '/server/pages/en/404.html' },
+      { force: false, from: '/test', status: 404, to: '/server/pages/en/404.html' },
+      { force: false, from: '/_next/data/test/es/test.json', status: 404, to: '/server/pages/es/404.html' },
+      { force: false, from: '/es/test', status: 404, to: '/server/pages/es/404.html' },
+      { force: false, from: '/_next/data/test/fr/test.json', status: 404, to: '/server/pages/fr/404.html' },
+      { force: false, from: '/fr/test', status: 404, to: '/server/pages/fr/404.html' },
+    ])
+  })
+})