remove HMR polling in favor of more targeted events (#54406)

This removes the client-side polling logic that we're doing on every HMR
tick in favor of events like `addedPage` / `removedPage`. It was leading
to a lot of misc issues & confusion surrounding error/404 pages being
polled needlessly.

Instead this updates `addedPage` / `removedPage` to be emitted on the
server, so that we can leverage those hooks to reload when necessary.

Fixes #10024
Fixes #51132
Closes NEXT-1372
related discussion: #40000

---------

packages/next/src/client/components/react-dev-overlay/hot-reloader-client.tsx

@@ -409,39 +409,6 @@ function processMessage(
       return
     }
     case 'pong': {
-      const { invalid } = obj
-      if (invalid) {
-        // Payload can be invalid even if the page does exist.
-        // So, we check if it can be created.
-        fetch(window.location.href, {
-          credentials: 'same-origin',
-        }).then((pageRes) => {
-          let shouldRefresh = pageRes.ok
-          // TODO-APP: investigate why edge runtime needs to reload
-          const isEdgeRuntime = pageRes.headers.get('x-edge-runtime') === '1'
-          if (pageRes.status === 404) {
-            // Check if head present as document.head could be null
-            // We are still on the page,
-            // dispatch an error so it's caught by the NotFound handler
-            const devErrorMetaTag = document.head?.querySelector(
-              'meta[name="next-error"]'
-            )
-            shouldRefresh = !devErrorMetaTag
-          }
-          // Page exists now, reload
-          startTransition(() => {
-            if (shouldRefresh) {
-              if (isEdgeRuntime) {
-                window.location.reload()
-              } else {
-                // @ts-ignore it exists, it's just hidden
-                router.fastRefresh()
-                dispatcher.onRefresh()
-              }
-            }
-          })
-        })
-      }
       return
     }
     case 'devPagesManifestUpdate': {

packages/next/src/client/dev/on-demand-entries-client.ts

@@ -1,5 +1,5 @@
 import Router from '../router'
-import { addMessageListener, sendMessage } from './error-overlay/websocket'
+import { sendMessage } from './error-overlay/websocket'
 
 export default async (page?: string) => {
   if (page) {
@@ -25,42 +25,4 @@ export default async (page?: string) => {
       }, 2500)
     })
   }
-
-  addMessageListener((event) => {
-    if (!event.data.includes('{')) return
-    try {
-      const payload = JSON.parse(event.data)
-      // don't attempt fetching the page if we're already showing
-      // the dev overlay as this can cause the error to be triggered
-      // repeatedly
-      if (
-        payload.event === 'pong' &&
-        payload.invalid &&
-        !self.__NEXT_DATA__.err
-      ) {
-        // Payload can be invalid even if the page does exist.
-        // So, we check if it can be created.
-        fetch(location.href, {
-          credentials: 'same-origin',
-        }).then((pageRes) => {
-          if (pageRes.status === 200) {
-            // Page exists now, reload
-            location.reload()
-          } else {
-            // Page doesn't exist
-            if (
-              self.__NEXT_DATA__.page === Router.pathname &&
-              Router.pathname !== '/_error'
-            ) {
-              // We are still on the page,
-              // reload to show 404 error page
-              location.reload()
-            }
-          }
-        })
-      }
-    } catch (err) {
-      console.error('on-demand-entries failed to parse response', err)
-    }
-  })
 }

packages/next/src/client/dev/webpack-hot-middleware-client.ts

@@ -5,6 +5,12 @@ export default () => {
   const devClient = connect()
 
   devClient.subscribeToHmrEvent((obj: any) => {
+    // if we're on an error/404 page, we can't reliably tell if the newly added/removed page
+    // matches the current path. In that case, assume any added/removed entries should trigger a reload of the current page
+    const isOnErrorPage =
+      window.next.router.pathname === '/404' ||
+      window.next.router.pathname === '/_error'
+
     if (obj.action === 'reloadPage') {
       sendMessage(
         JSON.stringify({
@@ -16,7 +22,7 @@ export default () => {
     }
     if (obj.action === 'removedPage') {
       const [page] = obj.data
-      if (page === window.next.router.pathname) {
+      if (page === window.next.router.pathname || isOnErrorPage) {
         sendMessage(
           JSON.stringify({
             event: 'client-removed-page',
@@ -31,8 +37,9 @@ export default () => {
     if (obj.action === 'addedPage') {
       const [page] = obj.data
       if (
-        page === window.next.router.pathname &&
-        typeof window.next.router.components[page] === 'undefined'
+        (page === window.next.router.pathname &&
+          typeof window.next.router.components[page] === 'undefined') ||
+        isOnErrorPage
       ) {
         sendMessage(
           JSON.stringify({

packages/next/src/server/dev/on-demand-entry-handler.ts

@@ -666,9 +666,6 @@ export function onDemandEntryHandler({
         continue
       }
 
-      // 404 is an on demand entry but when a new page is added we have to refresh the page
-      toSend = page === '/_error' ? { invalid: true } : { success: true }
-
       // We don't need to maintain active state of anything other than BUILT entries
       if (entryInfo.status !== BUILT) continue
 
@@ -683,6 +680,7 @@ export function onDemandEntryHandler({
       }
       entryInfo.lastActiveTime = Date.now()
       entryInfo.dispose = false
+      toSend = { success: true }
     }
     return toSend
   }

packages/next/src/server/lib/router-utils/setup-dev.ts

@@ -1280,10 +1280,25 @@ async function startWatcher(opts: SetupOpts) {
         opts.fsChecker.dynamicRoutes.unshift(...dataRoutes)
 
         if (!prevSortedRoutes?.every((val, idx) => val === sortedRoutes[idx])) {
+          const addedRoutes = sortedRoutes.filter(
+            (route) => !prevSortedRoutes.includes(route)
+          )
+          const removedRoutes = prevSortedRoutes.filter(
+            (route) => !sortedRoutes.includes(route)
+          )
+
           // emit the change so clients fetch the update
           hotReloader.send('devPagesManifestUpdate', {
             devPagesManifest: true,
           })
+
+          addedRoutes.forEach((route) => {
+            hotReloader.send('addedPage', route)
+          })
+
+          removedRoutes.forEach((route) => {
+            hotReloader.send('removedPage', route)
+          })
         }
         prevSortedRoutes = sortedRoutes
 

packages/next/src/server/next-server.ts

@@ -1856,8 +1856,10 @@ export default class NextNodeServer extends BaseServer {
         getRequestMeta(params.req, '_nextIncrementalCache'),
     })
 
-    params.res.statusCode = result.response.status
-    params.res.statusMessage = result.response.statusText
+    if (!params.res.statusCode || params.res.statusCode < 400) {
+      params.res.statusCode = result.response.status
+      params.res.statusMessage = result.response.statusText
+    }
 
     // TODO: (wyattjoh) investigate improving this
 

test/development/app-hmr/hmr.test.ts

@@ -1,5 +1,5 @@
 import { createNextDescribe } from 'e2e-utils'
-import { check } from 'next-test-utils'
+import { check, waitFor } from 'next-test-utils'
 
 const envFile = '.env.development.local'
 
@@ -10,6 +10,27 @@ createNextDescribe(
   },
   ({ next }) => {
     describe('filesystem changes', () => {
+      it('should not continously poll when hitting a not found page', async () => {
+        let requestCount = 0
+
+        const browser = await next.browser('/does-not-exist', {
+          beforePageLoad(page) {
+            page.on('request', (request) => {
+              const url = new URL(request.url())
+              if (url.pathname === '/does-not-exist') {
+                requestCount++
+              }
+            })
+          },
+        })
+        const body = await browser.elementByCss('body').text()
+        expect(body).toContain('404')
+
+        await waitFor(3000)
+
+        expect(requestCount).toBe(1)
+      })
+
       it('should not break when renaming a folder', async () => {
         const browser = await next.browser('/folder')
         const text = await browser.elementByCss('h1').text()

test/development/basic/hmr.test.ts

@@ -12,6 +12,7 @@ import {
 } from 'next-test-utils'
 import { createNext, FileRef } from 'e2e-utils'
 import { NextInstance } from 'test/lib/next-modes/base'
+import { outdent } from 'outdent'
 
 describe.each([[''], ['/docs']])(
   'basic HMR, basePath: %p',
@@ -410,6 +411,98 @@ describe.each([[''], ['/docs']])(
         }
       })
 
+      it('should recover from 404 after a page has been added with dynamic segments', async () => {
+        let browser
+        const newPage = join('pages', 'hmr', '[foo]', 'page.js')
+
+        try {
+          const start = next.cliOutput.length
+          browser = await webdriver(next.url, basePath + '/hmr/foo/page')
+
+          expect(await browser.elementByCss('body').text()).toMatch(
+            /This page could not be found/
+          )
+
+          // Add the page
+          await next.patchFile(
+            newPage,
+            'export default () => (<div id="new-page">the-new-page</div>)'
+          )
+
+          await check(() => getBrowserBodyText(browser), /the-new-page/)
+
+          await next.deleteFile(newPage)
+
+          await check(
+            () => getBrowserBodyText(browser),
+            /This page could not be found/
+          )
+
+          expect(next.cliOutput.slice(start)).toContain(
+            'compiling /hmr/[foo]/page (client and server)...'
+          )
+          expect(next.cliOutput).toContain(
+            'compiling /_error (client and server)...'
+          )
+        } catch (err) {
+          await next.deleteFile(newPage)
+          throw err
+        } finally {
+          if (browser) {
+            await browser.close()
+          }
+        }
+      })
+
+      it('should not continously poll a custom error page', async () => {
+        const errorPage = join('pages', '_error.js')
+
+        await next.patchFile(
+          errorPage,
+          outdent`
+          function Error({ statusCode, message, count }) {
+            return (
+              <div>
+                Error Message: {message}
+              </div>
+            )
+          }
+          
+          Error.getInitialProps = async ({ res, err }) => {
+            const statusCode = res ? res.statusCode : err ? err.statusCode : 404
+            console.log('getInitialProps called');
+            return {
+              statusCode,
+              message: err ? err.message : 'Oops...',
+            }
+          }
+          
+          export default Error          
+        `
+        )
+
+        const outputIndex = next.cliOutput.length
+        try {
+          // navigate to a 404 page
+          await webdriver(next.url, basePath + '/does-not-exist')
+
+          await check(
+            () => next.cliOutput.slice(outputIndex),
+            /getInitialProps called/
+          )
+
+          // wait a few seconds to ensure polling didn't happen
+          await waitFor(3000)
+
+          const logOccurrences =
+            next.cliOutput.slice(outputIndex).split('getInitialProps called')
+              .length - 1
+          expect(logOccurrences).toBe(1)
+        } finally {
+          await next.deleteFile(errorPage)
+        }
+      })
+
       it('should detect syntax errors and recover', async () => {
         let browser
         const aboutPage = join('pages', 'hmr', 'about2.js')