fix: correct redirect priority and correctly handle ISR pages assets (#826)

* fix: correct redirect sorting

* fix: don't move isr files

* chore: demo fixes

* fix: patch fs write methods

* fix: update tests

* fix: handle error and remove patch

* fix: disable isr disk cache

* fix: add isr warning

* chore: add comments

* fix: snapshot

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

Author: ascorbic

demo/pages/api/hello.js

@@ -1,5 +1,5 @@
 // Next.js API route support: https://nextjs.org/docs/api-routes/introduction
 
 export default (req, res) => {
-  res.status(200).json({ name: 'John Doe' })
+  res.status(200).json({ name: 'John Doe', query: req.query })
 }

demo/pages/getStaticProps/withRevalidate/[id].js

@@ -1,14 +1,14 @@
 import Link from 'next/link'
 
-const Show = ({ show }) => (
+const Show = ({ show, time }) => (
   <div>
     <p>This page uses getStaticProps() to pre-fetch a TV show.</p>
 
     <hr />
 
     <h1>Show #{show.id}</h1>
     <p>{show.name}</p>
-
+    <p>Rendered at {time}</p>
     <hr />
 
     <Link href="/">
@@ -32,10 +32,12 @@ export async function getStaticProps({ params }) {
 
   const res = await fetch(`https://api.tvmaze.com/shows/${id}`)
   const data = await res.json()
-
+  const time = new Date().toLocaleTimeString()
+  await new Promise((resolve) => setTimeout(resolve, 1000))
   return {
     props: {
       show: data,
+      time,
     },
     revalidate: 1,
   }

demo/pages/index.js

@@ -4,29 +4,18 @@ const Header = dynamic(() => import(/* webpackChunkName: 'header' */ '../compone
 import { useRouter } from 'next/router'
 
 const Index = ({ shows }) => {
-  const { locale } = useRouter();
+  const { locale } = useRouter()
 
   return (
     <div>
       <img src="/next-on-netlify.png" alt="NextJS on Netlify Banner" style={{ maxWidth: '100%' }} />
-      
-      <Header/>
+
+      <Header />
 
       <h1>NextJS on Netlify</h1>
-      <p>
-        This is a demo of a NextJS application with Server-Side Rendering (SSR).
-        <br />
-        It is hosted on Netlify.
-        <br />
-        Server-side rendering is handled by Netlify Functions.
-        <br />
-        Minimal configuration is required.
-        <br />
-        Everything is handled by the <a href="https://www.npmjs.com/package/next-on-netlify">next-on-netlify</a> npm
-        package.
-      </p>
+      <p>This is a demo of a NextJS application with Server-Side Rendering (SSR).</p>
 
-      <h2>1. Server-Side Rendering Made Easy</h2>
+      <h2>Server-Side Rendering</h2>
       <p>
         This page is server-side rendered.
         <br />
@@ -38,7 +27,7 @@ const Index = ({ shows }) => {
       <ul data-testid="list-server-side">
         {shows.map(({ id, name }) => (
           <li key={id}>
-            <Link href="/shows/[id]" as={`/shows/${id}`}>
+            <Link href={`/shows/${id}`}>
               <a>
                 #{id}: {name}
               </a>
@@ -47,17 +36,13 @@ const Index = ({ shows }) => {
         ))}
       </ul>
 
-      <h2>2. Full Support for Dynamic Pages</h2>
-      <p>
-        Dynamic pages, introduced in NextJS 9.2, are fully supported.
-        <br />
-        Click on a show to check out a server-side rendered page with dynamic routing (/shows/:id).
-      </p>
+      <h2>Dynamic Pages</h2>
+      <p>Click on a show to check out a server-side rendered page with dynamic routing (/shows/:id).</p>
 
       <ul data-testid="list-dynamic-pages">
         {shows.slice(0, 3).map(({ id, name }) => (
           <li key={id}>
-            <Link href="/shows/[id]" as={`/shows/${id}`}>
+            <Link href={`/shows/${id}`}>
               <a>
                 #{id}: {name}
               </a>
@@ -66,40 +51,27 @@ const Index = ({ shows }) => {
         ))}
       </ul>
 
-      <h2>3. Catch-All Routes? Included ✔</h2>
-      <p>
-        You can even take advantage of{' '}
-        <a href="https://nextjs.org/docs/routing/dynamic-routes#catch-all-routes">NextJS catch-all routes feature</a>
-        .
-        <br />
-        Here are three examples:
-      </p>
+      <h2>Catch-All Routess</h2>
+
       <ul data-testid="list-catch-all">
         <li>
-          <Link href="/shows/[...params]" as={`/shows/73/whatever/path/you/want`}>
+          <Link href={`/shows/73/whatever/path/you/want`}>
             <a>/shows/73/whatever/path/you/want</a>
           </Link>
         </li>
         <li>
-          <Link href="/shows/[...params]" as={`/shows/94/whatever/path/you`}>
+          <Link href={`/shows/94/whatever/path/you`}>
             <a>/shows/94/whatever/path/you</a>
           </Link>
         </li>
         <li>
-          <Link href="/shows/[...params]" as={`/shows/106/whatever/path`}>
+          <Link href={`/shows/106/whatever/path`}>
             <a>/shows/106/whatever/path</a>
           </Link>
         </li>
       </ul>
 
-      <h2>4. Static Pages Stay Static</h2>
-      <p>
-        next-on-netlify automatically determines which pages are dynamic and which ones are static.
-        <br />
-        Only dynamic pages are server-side rendered.
-        <br />
-        Static pages are pre-rendered and served directly by Netlify&apos;s CDN.
-      </p>
+      <h2>Static Pages</h2>
 
       <ul data-testid="list-static">
         <li>
@@ -108,49 +80,47 @@ const Index = ({ shows }) => {
           </Link>
         </li>
         <li>
-          <Link href="/static/[id]" as="/static/123456789">
+          <Link href="/static/123456789">
             <a>Static NextJS page with dynamic routing: /static/:id</a>
           </Link>
         </li>
       </ul>
 
-      <h2>5. Localization As Expected</h2>
+      <h2>Localization</h2>
       <p>
-        Localization (i18n) is supported! This demo uses <code>fr</code> with <code>en</code> as the default locale (at <code>/</code>).
+        Localization (i18n) is supported! This demo uses <code>fr</code> with <code>en</code> as the default locale (at{' '}
+        <code>/</code>).
       </p>
       <strong>The current locale is {locale}</strong>
       <p>Click on the links below to see the above text change</p>
       <ul data-testid="list-localization">
         <li>
           <Link href="/fr">
-              <a>/fr</a>
+            <a>/fr</a>
           </Link>
         </li>
         <li>
           <Link href="/en">
-              <a>/en (default locale)</a>
+            <a>/en (default locale)</a>
           </Link>
         </li>
       </ul>
-      
-      <h1>Want to Learn More?</h1>
-      <p>
-        Check out the <a href="https://github.com/FinnWoelm/next-on-netlify-demo">source code on GitHub</a>.
-      </p>
     </div>
   )
 }
 
 Index.getInitialProps = async function () {
-  const dev = process.env.CONTEXT !== 'production';
+  const dev = process.env.CONTEXT !== 'production'
 
   // Set a random page between 1 and 100
   const randomPage = Math.floor(Math.random() * 100) + 1
   // FIXME: stub out in dev
-  const server = dev ? `https://api.tvmaze.com/shows?page=${randomPage}` : `https://api.tvmaze.com/shows?page=${randomPage}`;
+  const server = dev
+    ? `https://api.tvmaze.com/shows?page=${randomPage}`
+    : `https://api.tvmaze.com/shows?page=${randomPage}`
 
   // Get the data
-  const res = await fetch(server);
+  const res = await fetch(server)
   const data = await res.json()
 
   return { shows: data.slice(0, 5) }

src/helpers/config.js

@@ -1,5 +1,8 @@
-// @ts-check
+/* eslint-disable max-lines */
+
+const { yellowBright } = require('chalk')
 const { readJSON, existsSync } = require('fs-extra')
+const { outdent } = require('outdent')
 const { join, dirname, relative } = require('pathe')
 const slash = require('slash')
 
@@ -52,9 +55,9 @@ const getNetlifyRoutes = (nextRoute) => {
 }
 
 exports.generateRedirects = async ({ netlifyConfig, basePath, i18n }) => {
-  const { dynamicRoutes } = await readJSON(join(netlifyConfig.build.publish, 'prerender-manifest.json'))
-
-  const redirects = []
+  const { dynamicRoutes, routes: staticRoutes } = await readJSON(
+    join(netlifyConfig.build.publish, 'prerender-manifest.json'),
+  )
 
   netlifyConfig.redirects.push(
     ...HIDDEN_PATHS.map((path) => ({
@@ -65,15 +68,35 @@ exports.generateRedirects = async ({ netlifyConfig, basePath, i18n }) => {
     })),
   )
 
+  const dataRedirects = []
+  const pageRedirects = []
+  const isrRedirects = []
+  let hasIsr = false
+
   const dynamicRouteEntries = Object.entries(dynamicRoutes)
-  dynamicRouteEntries.sort((a, b) => a[0].localeCompare(b[0]))
+
+  const staticRouteEntries = Object.entries(staticRoutes)
+
+  staticRouteEntries.forEach(([route, { dataRoute, initialRevalidateSeconds }]) => {
+    // Only look for revalidate as we need to rewrite these to SSR rather than ODB
+    if (initialRevalidateSeconds === false) {
+      // These can be ignored, as they're static files handled by the CDN
+      return
+    }
+    if (i18n.defaultLocale && route.startsWith(`/${i18n.defaultLocale}/`)) {
+      route = route.slice(i18n.defaultLocale.length + 1)
+    }
+    hasIsr = true
+    isrRedirects.push(...getNetlifyRoutes(dataRoute), ...getNetlifyRoutes(route))
+  })
 
   dynamicRouteEntries.forEach(([route, { dataRoute, fallback }]) => {
     // Add redirects if fallback is "null" (aka blocking) or true/a string
     if (fallback === false) {
       return
     }
-    redirects.push(...getNetlifyRoutes(route), ...getNetlifyRoutes(dataRoute))
+    pageRedirects.push(...getNetlifyRoutes(route))
+    dataRedirects.push(...getNetlifyRoutes(dataRoute))
   })
 
   if (i18n) {
@@ -82,21 +105,57 @@ exports.generateRedirects = async ({ netlifyConfig, basePath, i18n }) => {
 
   // This is only used in prod, so dev uses `next dev` directly
   netlifyConfig.redirects.push(
+    // Static files are in `static`
     { from: `${basePath}/_next/static/*`, to: `/static/:splat`, status: 200 },
+    // API routes always need to be served from the regular function
+    {
+      from: `${basePath}/api`,
+      to: HANDLER_FUNCTION_PATH,
+      status: 200,
+    },
+    {
+      from: `${basePath}/api/*`,
+      to: HANDLER_FUNCTION_PATH,
+      status: 200,
+    },
+    // Preview mode gets forced to the function, to bypess pre-rendered pages
     {
       from: `${basePath}/*`,
       to: HANDLER_FUNCTION_PATH,
       status: 200,
       conditions: { Cookie: ['__prerender_bypass', '__next_preview_data'] },
       force: true,
     },
-    ...redirects.map((redirect) => ({
+    // ISR redirects are handled by the regular function. Forced to avoid pre-rendered pages
+    ...isrRedirects.map((redirect) => ({
+      from: `${basePath}${redirect}`,
+      to: HANDLER_FUNCTION_PATH,
+      status: 200,
+      force: true,
+    })),
+    // These are pages with fallback set, which need an ODB
+    // Data redirects go first, to avoid conflict with splat redirects
+    ...dataRedirects.map((redirect) => ({
       from: `${basePath}${redirect}`,
       to: ODB_FUNCTION_PATH,
       status: 200,
     })),
+    // ...then all the other fallback pages
+    ...pageRedirects.map((redirect) => ({
+      from: `${basePath}${redirect}`,
+      to: ODB_FUNCTION_PATH,
+      status: 200,
+    })),
+    // Everything else is handled by the regular function
     { from: `${basePath}/*`, to: HANDLER_FUNCTION_PATH, status: 200 },
   )
+  if (hasIsr) {
+    console.log(
+      yellowBright(outdent`
+        You have some pages that use ISR (pages that use getStaticProps with revalidate set), which is not currently fully-supported by this plugin. Be aware that results may be unreliable.
+      `),
+    )
+  }
 }
 
 exports.getNextConfig = async function getNextConfig({ publish, failBuild = defaultFailBuild }) {
@@ -159,3 +218,4 @@ exports.configureHandlerFunctions = ({ netlifyConfig, publish, ignore = [] }) =>
     })
   })
 }
+/* eslint-enable max-lines */

src/helpers/files.js

@@ -47,12 +47,29 @@ exports.moveStaticPages = async ({ netlifyConfig, target, i18n }) => {
     }
   }
 
+  const prerenderManifest = await readJson(join(netlifyConfig.build.publish, 'prerender-manifest.json'))
+
+  const isrFiles = new Set()
+
+  Object.entries(prerenderManifest.routes).forEach(([route, { initialRevalidateSeconds }]) => {
+    if (initialRevalidateSeconds) {
+      // Find all files used by ISR routes
+      const trimmedPath = route.slice(1)
+      isrFiles.add(`${trimmedPath}.html`)
+      isrFiles.add(`${trimmedPath}.json`)
+    }
+  })
+
   const files = []
   const moveFile = async (file) => {
     const source = join(root, file)
     files.push(file)
     const dest = join(netlifyConfig.build.publish, file)
-    await move(source, dest)
+    try {
+      await move(source, dest)
+    } catch (error) {
+      console.warn('Error moving file', source, error)
+    }
   }
   // Move all static files, except error documents and nft manifests
   const pages = await globby(['**/*.{html,json}', '!**/(500|404|*.js.nft).{html,json}'], {
@@ -67,6 +84,10 @@ exports.moveStaticPages = async ({ netlifyConfig, target, i18n }) => {
   const limit = pLimit(Math.max(2, cpus().length))
   const promises = pages.map(async (rawPath) => {
     const filePath = slash(rawPath)
+    // Don't move ISR files, as they're used for the first request
+    if (isrFiles.has(filePath)) {
+      return
+    }
     if (isDynamicRoute(filePath)) {
       return
     }