Ensure dev server side errors are correct (#28520)

ijjk · web-flow · commit f1dbc9260d48 · 2021-08-27T14:29:30.000+02:00
diff --git a/packages/next/client/dev/on-demand-entries-client.js b/packages/next/client/dev/on-demand-entries-client.js
@@ -9,7 +9,11 @@ export default async ({ assetPrefix }) => {
     )
   })
 
-  setupPing(assetPrefix, () => Router.pathname, currentPage)
+  setupPing(
+    assetPrefix,
+    () => Router.query.__NEXT_PAGE || Router.pathname,
+    currentPage
+  )
 
   // prevent HMR connection from being closed when running tests
   if (!process.env.__NEXT_TEST_MODE) {
diff --git a/packages/next/client/dev/on-demand-entries-utils.js b/packages/next/client/dev/on-demand-entries-utils.js
@@ -29,7 +29,10 @@ export function setupPing(assetPrefix, pathnameFn, retry) {
     if (event.data.indexOf('{') === -1) return
     try {
       const payload = JSON.parse(event.data)
-      if (payload.invalid) {
+      // 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.invalid && !self.__NEXT_DATA__.err) {
         // Payload can be invalid even if the page does not exist.
         // So, we need to make sure it exists before reloading.
         fetch(location.href, {
diff --git a/packages/next/client/next-dev.js b/packages/next/client/next-dev.js
@@ -59,6 +59,12 @@ initNext({ webpackHMR })
       } else if (event.data.indexOf('serverOnlyChanges') !== -1) {
         const { pages } = JSON.parse(event.data)
 
+        // Make sure to reload when the dev-overlay is showing for an
+        // API route
+        if (pages.includes(router.query.__NEXT_PAGE)) {
+          return window.location.reload()
+        }
+
         if (!router.clc && pages.includes(router.pathname)) {
           console.log('Refreshing page data due to server-side change')
 
diff --git a/packages/next/server/api-utils.ts b/packages/next/server/api-utils.ts
@@ -25,7 +25,9 @@ export async function apiResolver(
   query: any,
   resolverModule: any,
   apiContext: __ApiPreviewProps,
-  propagateError: boolean
+  propagateError: boolean,
+  dev?: boolean,
+  page?: string
 ): Promise<void> {
   const apiReq = req as NextApiRequest
   const apiRes = res as NextApiResponse
@@ -117,6 +119,13 @@ export async function apiResolver(
     if (err instanceof ApiError) {
       sendError(apiRes, err.statusCode, err.message)
     } else {
+      if (dev) {
+        if (err) {
+          err.page = page
+        }
+        throw err
+      }
+
       console.error(err)
       if (propagateError) {
         throw err
diff --git a/packages/next/server/dev/hot-reloader.ts b/packages/next/server/dev/hot-reloader.ts
@@ -134,7 +134,7 @@ export default class HotReloader {
   private webpackHotMiddleware: (NextHandleFunction & any) | null
   private config: NextConfigComplete
   private stats: webpack.Stats | null
-  private serverStats: webpack.Stats | null
+  public serverStats: webpack.Stats | null
   private clientError: Error | null = null
   private serverError: Error | null = null
   private serverPrevDocumentHash: string | null
diff --git a/packages/next/server/dev/next-dev-server.ts b/packages/next/server/dev/next-dev-server.ts
@@ -1,5 +1,6 @@
 import crypto from 'crypto'
 import fs from 'fs'
+import chalk from 'chalk'
 import { IncomingMessage, ServerResponse } from 'http'
 import { Worker } from 'jest-worker'
 import AmpHtmlValidator from 'next/dist/compiled/amphtml-validator'
@@ -47,6 +48,12 @@ import {
   loadDefaultErrorComponents,
 } from '../load-components'
 import { DecodeError } from '../../shared/lib/utils'
+import { parseStack } from '@next/react-dev-overlay/lib/internal/helpers/parseStack'
+import {
+  createOriginalStackFrame,
+  getSourceById,
+} from '@next/react-dev-overlay/lib/middleware'
+import * as Log from '../../build/output/log'
 
 // Load ReactDevOverlay only when needed
 let ReactDevOverlayImpl: React.FunctionComponent
@@ -325,6 +332,15 @@ export default class DevServer extends Server {
     )
     // This is required by the tracing subsystem.
     setGlobal('telemetry', telemetry)
+
+    process.on('unhandledRejection', (reason) => {
+      this.logErrorWithOriginalStack(reason, 'unhandledRejection').catch(
+        () => {}
+      )
+    })
+    process.on('uncaughtException', (err) => {
+      this.logErrorWithOriginalStack(err, 'uncaughtException').catch(() => {})
+    })
   }
 
   protected async close(): Promise<void> {
@@ -431,8 +447,85 @@ export default class DevServer extends Server {
       // if they should match against the basePath or not
       parsedUrl.pathname = originalPathname
     }
+    try {
+      return await super.run(req, res, parsedUrl)
+    } catch (err) {
+      res.statusCode = 500
+      try {
+        this.logErrorWithOriginalStack(err).catch(() => {})
+        return await this.renderError(err, req, res, pathname!, {
+          __NEXT_PAGE: err?.page || pathname,
+        })
+      } catch (internalErr) {
+        console.error(internalErr)
+        res.end('Internal Server Error')
+      }
+    }
+  }
 
-    return super.run(req, res, parsedUrl)
+  private async logErrorWithOriginalStack(
+    possibleError?: any,
+    type?: 'unhandledRejection' | 'uncaughtException'
+  ) {
+    let usedOriginalStack = false
+
+    if (possibleError?.name && possibleError?.stack && possibleError?.message) {
+      const err: Error & { stack: string } = possibleError
+      try {
+        const frames = parseStack(err.stack)
+        const frame = frames[0]
+
+        if (frame.lineNumber && frame?.file) {
+          const compilation = this.hotReloader?.serverStats?.compilation
+          const moduleId = frame.file!.replace(
+            /^(webpack-internal:\/\/\/|file:\/\/)/,
+            ''
+          )
+
+          const source = await getSourceById(
+            !!frame.file?.startsWith(sep) || !!frame.file?.startsWith('file:'),
+            moduleId,
+            compilation,
+            this.hotReloader!.isWebpack5
+          )
+
+          const originalFrame = await createOriginalStackFrame({
+            line: frame.lineNumber!,
+            column: frame.column,
+            source,
+            frame,
+            modulePath: moduleId,
+            rootDirectory: this.dir,
+          })
+
+          if (originalFrame) {
+            const { originalCodeFrame, originalStackFrame } = originalFrame
+            const { file, lineNumber, column, methodName } = originalStackFrame
+
+            console.error(
+              chalk.red('error') +
+                ' - ' +
+                `${file} (${lineNumber}:${column}) @ ${methodName}`
+            )
+            console.error(`${chalk.red(err.name)}: ${err.message}`)
+            console.error(originalCodeFrame)
+            usedOriginalStack = true
+          }
+        }
+      } catch (_) {
+        // failed to load original stack using source maps
+        // this un-actionable by users so we don't show the
+        // internal error and only show the provided stack
+      }
+    }
+
+    if (!usedOriginalStack) {
+      if (type) {
+        Log.error(`${type}:`, possibleError)
+      } else {
+        Log.error(possibleError)
+      }
+    }
   }
 
   // override production loading of routes-manifest
diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts
@@ -487,7 +487,7 @@ export default class Server {
     try {
       return await this.run(req, res, parsedUrl)
     } catch (err) {
-      if (this.minimalMode) {
+      if (this.minimalMode || this.renderOpts.dev) {
         throw err
       }
       this.logError(err)
@@ -1125,7 +1125,9 @@ export default class Server {
       query,
       pageModule,
       this.renderOpts.previewProps,
-      this.minimalMode
+      this.minimalMode,
+      this.renderOpts.dev,
+      page
     )
     return true
   }
@@ -1857,6 +1859,7 @@ export default class Server {
     ctx: RequestContext
   ): Promise<ResponsePayload | null> {
     const { res, query, pathname } = ctx
+    let page = pathname
     const bubbleNoFallback = !!query._nextBubbleNoFallback
     delete query._nextBubbleNoFallback
 
@@ -1888,6 +1891,7 @@ export default class Server {
           )
           if (dynamicRouteResult) {
             try {
+              page = dynamicRoute.page
               return await this.renderToResponseWithComponents(
                 {
                   ...ctx,
@@ -1929,7 +1933,10 @@ export default class Server {
       )
 
       if (!isWrappedError) {
-        if (this.minimalMode) {
+        if (this.minimalMode || this.renderOpts.dev) {
+          if (err) {
+            err.page = page
+          }
           throw err
         }
         this.logError(err)
diff --git a/packages/next/server/render.tsx b/packages/next/server/render.tsx
@@ -822,10 +822,7 @@ export async function renderToHTML(
       ;(renderOpts as any).pageData = props
     }
   } catch (dataFetchError) {
-    if (isDataReq || !dev || !dataFetchError) throw dataFetchError
-    ctx.err = dataFetchError
-    renderOpts.err = dataFetchError
-    console.error(dataFetchError)
+    throw dataFetchError
   }
 
   if (
diff --git a/packages/react-dev-overlay/src/middleware.ts b/packages/react-dev-overlay/src/middleware.ts
@@ -177,52 +177,50 @@ export async function createOriginalStackFrame({
   }
 }
 
-function getOverlayMiddleware(options: OverlayMiddlewareOptions) {
-  async function getSourceById(
-    isServerSide: boolean,
-    isFile: boolean,
-    id: string
-  ): Promise<Source> {
-    if (isFile) {
-      const fileContent: string | null = await fs
-        .readFile(id, 'utf-8')
-        .catch(() => null)
-
-      if (fileContent == null) {
-        return null
-      }
-
-      const map = getRawSourceMap(fileContent)
-      if (map == null) {
-        return null
-      }
+export async function getSourceById(
+  isFile: boolean,
+  id: string,
+  compilation: any,
+  isWebpack5: boolean
+): Promise<Source> {
+  if (isFile) {
+    const fileContent: string | null = await fs
+      .readFile(id, 'utf-8')
+      .catch(() => null)
+
+    if (fileContent == null) {
+      return null
+    }
 
-      return {
-        map() {
-          return map
-        },
-      }
+    const map = getRawSourceMap(fileContent)
+    if (map == null) {
+      return null
     }
 
-    try {
-      const compilation = isServerSide
-        ? options.serverStats()?.compilation
-        : options.stats()?.compilation
-      if (compilation == null) {
-        return null
-      }
+    return {
+      map() {
+        return map
+      },
+    }
+  }
 
-      const module = [...compilation.modules].find(
-        (searchModule) =>
-          getModuleId(compilation, searchModule, options.isWebpack5) === id
-      )
-      return getModuleSource(compilation, module, options.isWebpack5)
-    } catch (err) {
-      console.error(`Failed to lookup module by ID ("${id}"):`, err)
+  try {
+    if (compilation == null) {
       return null
     }
+
+    const module = [...compilation.modules].find(
+      (searchModule) =>
+        getModuleId(compilation, searchModule, isWebpack5) === id
+    )
+    return getModuleSource(compilation, module, isWebpack5)
+  } catch (err) {
+    console.error(`Failed to lookup module by ID ("${id}"):`, err)
+    return null
   }
+}
 
+function getOverlayMiddleware(options: OverlayMiddlewareOptions) {
   return async function (
     req: IncomingMessage,
     res: ServerResponse,
@@ -254,10 +252,15 @@ function getOverlayMiddleware(options: OverlayMiddlewareOptions) {
 
       let source: Source
       try {
+        const compilation = isServerSide
+          ? options.serverStats()?.compilation
+          : options.stats()?.compilation
+
         source = await getSourceById(
-          isServerSide,
           frame.file.startsWith('file:'),
-          moduleId
+          moduleId,
+          compilation,
+          !!options.isWebpack5
         )
       } catch (err) {
         console.log('Failed to get source map:', err)
diff --git a/test/integration/api-support/test/index.test.js b/test/integration/api-support/test/index.test.js
@@ -109,14 +109,24 @@ function runTests(dev = false) {
     const res = await fetchViaHTTP(appPort, '/api/user-error', null, {})
     const text = await res.text()
     expect(res.status).toBe(500)
-    expect(text).toBe('Internal Server Error')
+
+    if (dev) {
+      expect(text).toContain('User error')
+    } else {
+      expect(text).toBe('Internal Server Error')
+    }
   })
 
   it('should throw Internal Server Error (async)', async () => {
     const res = await fetchViaHTTP(appPort, '/api/user-error-async', null, {})
     const text = await res.text()
     expect(res.status).toBe(500)
-    expect(text).toBe('Internal Server Error')
+
+    if (dev) {
+      expect(text).toContain('User error')
+    } else {
+      expect(text).toBe('Internal Server Error')
+    }
   })
 
   it('should parse JSON body', async () => {
diff --git a/test/integration/server-side-dev-errors/pages/api/blog/[slug].js b/test/integration/server-side-dev-errors/pages/api/blog/[slug].js
@@ -0,0 +1,3 @@
+export default function handler(req, res) {
+  res.status(200).json({ slug: req.query.slug })
+}
diff --git a/test/integration/server-side-dev-errors/pages/api/hello.js b/test/integration/server-side-dev-errors/pages/api/hello.js
@@ -0,0 +1,3 @@
+export default function handler(req, res) {
+  res.status(200).json({ hello: 'world' })
+}
diff --git a/test/integration/server-side-dev-errors/pages/blog/[slug].js b/test/integration/server-side-dev-errors/pages/blog/[slug].js
diff --git a/test/integration/server-side-dev-errors/pages/gsp.js b/test/integration/server-side-dev-errors/pages/gsp.js
diff --git a/test/integration/server-side-dev-errors/pages/gssp.js b/test/integration/server-side-dev-errors/pages/gssp.js
diff --git a/test/integration/server-side-dev-errors/pages/uncaught-exception.js b/test/integration/server-side-dev-errors/pages/uncaught-exception.js
diff --git a/test/integration/server-side-dev-errors/pages/uncaught-rejection.js b/test/integration/server-side-dev-errors/pages/uncaught-rejection.js
diff --git a/test/integration/server-side-dev-errors/test/index.test.js b/test/integration/server-side-dev-errors/test/index.test.js

Original file line number	Diff line number	Diff line change
`@@ -822,10 +822,7 @@ export async function renderToHTML(`
`822`	`822`	`;(renderOpts as any).pageData = props`
`823`	`823`	`}`
`824`	`824`	`} catch (dataFetchError) {`
`825`		`- if (isDataReq \|\| !dev \|\| !dataFetchError) throw dataFetchError`
`826`		`- ctx.err = dataFetchError`
`827`		`- renderOpts.err = dataFetchError`
`828`		`- console.error(dataFetchError)`
	`825`	`+ throw dataFetchError`
`829`	`826`	`}`
`830`	`827`
`831`	`828`	`if (`