Development: Remove matchers.reload() on each request (#83829)

## What?

Re-apply #83720. It was reverted because of tests flaking significantly
more often.

Dug into this with @mischnic and @sokra yesterday and we found that it's
a race condition because of the two separate watchers. In
`setup-dev-bundler.ts` there's a Watchpack instance and Turbopack has
it's own watcher. The specific case where it caused flakes is when the
watchpack aggregate event runs before Turbopack's watcher has emitted a
change event. Since the watchpack aggregate callback emits `ADDED_PAGE`
/ `REMOVED_PAGE` over the websocket it would end up triggering the
browser `location.reload()` too early, before the Turbopack side has the
updated list of routes.

The right solution is to move the `matchers.reload()` and `ADDED_PAGE` /
`REMOVED_PAGE` events to where Turbopack emits that the list of routes
(entrypoints) has updated.

Author: timneutkens

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

@@ -606,6 +606,10 @@ export default abstract class Server<
     this.responseCache = this.getResponseCache({ dev })
   }
 
+  protected reloadMatchers() {
+    return this.matchers.reload()
+  }
+
   private handleRSCRequest: RouteHandler<ServerRequest, ServerResponse> = (
     req,
     _res,

packages/next/src/server/dev/hot-reloader-turbopack.ts

@@ -612,6 +612,19 @@ export async function createHotReloaderTurbopack(
         )
       }
 
+      const existingRoutes = [
+        ...currentEntrypoints.app.keys(),
+        ...currentEntrypoints.page.keys(),
+      ]
+      const newRoutes = [...entrypoints.routes.keys()]
+
+      const addedRoutes = newRoutes.filter(
+        (route) => !existingRoutes.includes(route)
+      )
+      const removedRoutes = existingRoutes.filter(
+        (route) => !newRoutes.includes(route)
+      )
+
       processTopLevelIssues(currentTopLevelIssues, entrypoints)
 
       await handleEntrypoints({
@@ -647,6 +660,35 @@ export async function createHotReloaderTurbopack(
         },
       })
 
+      // Reload matchers when the files have been compiled
+      await propagateServerField(opts, 'reloadMatchers', undefined)
+
+      if (addedRoutes.length > 0 || removedRoutes.length > 0) {
+        // When the list of routes changes a new manifest should be fetched for Pages Router.
+        hotReloader.send({
+          type: HMR_MESSAGE_SENT_TO_BROWSER.DEV_PAGES_MANIFEST_UPDATE,
+          data: [
+            {
+              devPagesManifest: true,
+            },
+          ],
+        })
+      }
+
+      for (const route of addedRoutes) {
+        hotReloader.send({
+          type: HMR_MESSAGE_SENT_TO_BROWSER.ADDED_PAGE,
+          data: [route],
+        })
+      }
+
+      for (const route of removedRoutes) {
+        hotReloader.send({
+          type: HMR_MESSAGE_SENT_TO_BROWSER.REMOVED_PAGE,
+          data: [route],
+        })
+      }
+
       currentEntriesHandlingResolve!()
       currentEntriesHandlingResolve = undefined
     }

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

@@ -1045,37 +1045,49 @@ async function startWatcher(
         }
         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)
-          )
+        // For Turbopack ADDED_PAGE and REMOVED_PAGE are implemented in hot-reloader-turbopack.ts
+        // in order to avoid a race condition where ADDED_PAGE and REMOVED_PAGE are sent before Turbopack picked up the file change.
+        if (!opts.turbo) {
+          // Reload the matchers. The filesystem would have been written to,
+          // and the matchers need to re-scan it to update the router.
+          // Reloading the matchers should happen before `ADDED_PAGE` or `REMOVED_PAGE` is sent over the websocket
+          // otherwise it sends the event too early.
+          await propagateServerField(opts, 'reloadMatchers', undefined)
 
-          // emit the change so clients fetch the update
-          hotReloader.send({
-            type: HMR_MESSAGE_SENT_TO_BROWSER.DEV_PAGES_MANIFEST_UPDATE,
-            data: [
-              {
-                devPagesManifest: true,
-              },
-            ],
-          })
+          if (
+            !prevSortedRoutes?.every((val, idx) => val === sortedRoutes[idx])
+          ) {
+            const addedRoutes = sortedRoutes.filter(
+              (route) => !prevSortedRoutes.includes(route)
+            )
+            const removedRoutes = prevSortedRoutes.filter(
+              (route) => !sortedRoutes.includes(route)
+            )
 
-          addedRoutes.forEach((route) => {
+            // emit the change so clients fetch the update
             hotReloader.send({
-              type: HMR_MESSAGE_SENT_TO_BROWSER.ADDED_PAGE,
-              data: [route],
+              type: HMR_MESSAGE_SENT_TO_BROWSER.DEV_PAGES_MANIFEST_UPDATE,
+              data: [
+                {
+                  devPagesManifest: true,
+                },
+              ],
             })
-          })
 
-          removedRoutes.forEach((route) => {
-            hotReloader.send({
-              type: HMR_MESSAGE_SENT_TO_BROWSER.REMOVED_PAGE,
-              data: [route],
+            addedRoutes.forEach((route) => {
+              hotReloader.send({
+                type: HMR_MESSAGE_SENT_TO_BROWSER.ADDED_PAGE,
+                data: [route],
+              })
             })
-          })
+
+            removedRoutes.forEach((route) => {
+              hotReloader.send({
+                type: HMR_MESSAGE_SENT_TO_BROWSER.REMOVED_PAGE,
+                data: [route],
+              })
+            })
+          }
         }
         prevSortedRoutes = sortedRoutes
 
@@ -1114,10 +1126,6 @@ async function startWatcher(
         } else {
           Log.warn('Failed to reload dynamic routes:', e)
         }
-      } finally {
-        // Reload the matchers. The filesystem would have been written to,
-        // and the matchers need to re-scan it to update the router.
-        await propagateServerField(opts, 'reloadMatchers', undefined)
       }
     })
 

packages/next/src/server/route-matcher-managers/dev-route-matcher-manager.ts

@@ -65,10 +65,6 @@ export class DevRouteMatcherManager extends DefaultRouteMatcherManager {
     pathname: string,
     options: MatchOptions
   ): AsyncGenerator<RouteMatch<RouteDefinition<RouteKind>>, null, undefined> {
-    // Compile the development routes.
-    // TODO: we may want to only run this during testing, users won't be fast enough to require this many dir scans
-    await super.reload()
-
     // Iterate over the development matches to see if one of them match the
     // request path.
     for await (const developmentMatch of super.matchAll(pathname, options)) {

test/integration/custom-error/test/index.test.js

@@ -10,6 +10,7 @@ import {
   killApp,
   launchApp,
   retry,
+  waitFor,
 } from 'next-test-utils'
 
 const appDir = join(__dirname, '..')
@@ -47,6 +48,8 @@ describe('Custom _error', () => {
         })
       } finally {
         await fs.remove(page404)
+        // Matches `next-dev.ts` patchFileDelay
+        await waitFor(1000)
       }
     })
   })
@@ -76,6 +79,8 @@ describe('Custom _error', () => {
         })
       } finally {
         await fs.remove(page404)
+        // Matches `next-dev.ts` patchFileDelay
+        await waitFor(1000)
       }
     })