Append owner stack for captured string console error (#72115)

huozhi · web-flow · commit 2ee93c34837c · 2024-11-06T01:20:52.000+01:00
diff --git a/packages/next/src/client/components/react-dev-overlay/internal/helpers/stitched-error.ts b/packages/next/src/client/components/react-dev-overlay/internal/helpers/stitched-error.ts
@@ -6,6 +6,10 @@ const REACT_ERROR_STACK_BOTTOM_FRAME_REGEX = new RegExp(
   `(at ${REACT_ERROR_STACK_BOTTOM_FRAME} )|(${REACT_ERROR_STACK_BOTTOM_FRAME}\\@)`
 )
 
+const captureOwnerStack = (React as any).captureOwnerStack
+  ? (React as any).captureOwnerStack
+  : () => ''
+
 export function getReactStitchedError<T = unknown>(err: T): Error | T {
   if (typeof (React as any).captureOwnerStack !== 'function') {
     return err
@@ -28,12 +32,18 @@ export function getReactStitchedError<T = unknown>(err: T): Error | T {
   newError.stack = newStack
 
   // Avoid duplicate overriding stack frames
-  const ownerStack = (React as any).captureOwnerStack()
-  if (ownerStack && newStack.endsWith(ownerStack) === false) {
-    newStack += ownerStack
-    // Override stack
-    newError.stack = newStack
-  }
+  appendOwnerStack(newError)
 
   return newError
 }
+
+function appendOwnerStack(error: Error) {
+  let stack = error.stack || ''
+  // Avoid duplicate overriding stack frames
+  const ownerStack = captureOwnerStack()
+  if (ownerStack && stack.endsWith(ownerStack) === false) {
+    stack += ownerStack
+    // Override stack
+    error.stack = stack
+  }
+}
diff --git a/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts b/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts
@@ -6,6 +6,7 @@ import { formatConsoleArgs } from '../../../../lib/console'
 import isError from '../../../../../lib/is-error'
 import { createUnhandledError } from './console-error'
 import { enqueueConsecutiveDedupedError } from './enqueue-client-error'
+import { getReactStitchedError } from './stitched-error'
 
 export type ErrorHandler = (error: Error) => void
 
@@ -22,7 +23,7 @@ export function handleClientError(
   if (!originError || !isError(originError)) {
     // If it's not an error, format the args into an error
     const formattedErrorMessage = formatConsoleArgs(consoleErrorArgs)
-    error = createUnhandledError(formattedErrorMessage)
+    error = getReactStitchedError(createUnhandledError(formattedErrorMessage))
   } else {
     error = originError
   }
diff --git a/test/development/app-dir/owner-stack-invalid-element-type/owner-stack-invalid-element-type.test.ts b/test/development/app-dir/owner-stack-invalid-element-type/owner-stack-invalid-element-type.test.ts
@@ -41,9 +41,10 @@ const isOwnerStackEnabled =
             11 | }"
         `)
       } else {
+        // FIXME: the owner stack method names should be `Inner > Page`
         expect(stackFramesContent).toMatchInlineSnapshot(`
-            "at BrowserOnly (app/browser/page.js (11:11))
-            at Inner (app/browser/page.js (15:11))"
+          "at BrowserOnly (app/browser/page.js (11:11))
+          at Inner (app/browser/page.js (15:11))"
         `)
         // FIXME: the methodName should be `@ BrowserOnly` instead of `@ Foo`
         expect(source).toMatchInlineSnapshot(`
@@ -83,11 +84,11 @@ const isOwnerStackEnabled =
             8 | export default function Page() {"
         `)
       } else {
-        // FIXME: the methodName should be `@ Page` instead of `@ Inner`
+        // FIXME: the owner stack method names should be `Page`
         expect(stackFramesContent).toMatchInlineSnapshot(
           `"at Inner (app/rsc/page.js (11:8))"`
         )
-        // FIXME: the methodName should be `@ Inner` instead of `@ Foo`
+        // FIXME: the methodName should be `@ Inner`
         expect(source).toMatchInlineSnapshot(`
           "app/rsc/page.js (5:11) @ Foo
 
@@ -125,11 +126,11 @@ const isOwnerStackEnabled =
             10 | export default function Page() {"
         `)
       } else {
-        // FIXME: the methodName should be `@ Inner` instead of `@ Foo`
+        // FIXME: the owner stack method names should be `Page`
         expect(stackFramesContent).toMatchInlineSnapshot(
           `"at Inner (app/ssr/page.js (13:8))"`
         )
-        // FIXME: the methodName should be `@ Inner` instead of `@ Foo`
+        // FIXME: the methodName should be `@ Inner`
         expect(source).toMatchInlineSnapshot(`
           "app/ssr/page.js (7:11) @ Foo
 
diff --git a/test/development/app-dir/owner-stack-react-missing-key-prop/app/layout.tsx b/test/development/app-dir/owner-stack-react-missing-key-prop/app/layout.tsx
@@ -0,0 +1,8 @@
+import { ReactNode } from 'react'
+export default function Root({ children }: { children: ReactNode }) {
+  return (
+    <html>
+      <body>{children}</body>
+    </html>
+  )
+}
diff --git a/test/development/app-dir/owner-stack-react-missing-key-prop/app/rsc/page.tsx b/test/development/app-dir/owner-stack-react-missing-key-prop/app/rsc/page.tsx
@@ -0,0 +1,11 @@
+const list = [1, 2, 3]
+
+export default function Page() {
+  return (
+    <div>
+      {list.map((item, index) => (
+        <span>{item}</span>
+      ))}
+    </div>
+  )
+}
diff --git a/test/development/app-dir/owner-stack-react-missing-key-prop/app/ssr/page.tsx b/test/development/app-dir/owner-stack-react-missing-key-prop/app/ssr/page.tsx
@@ -0,0 +1,13 @@
+'use client'
+
+const list = [1, 2, 3]
+
+export default function Page() {
+  return (
+    <div>
+      {list.map((item, index) => (
+        <p>{item}</p>
+      ))}
+    </div>
+  )
+}
diff --git a/test/development/app-dir/owner-stack-react-missing-key-prop/next.config.js b/test/development/app-dir/owner-stack-react-missing-key-prop/next.config.js
@@ -0,0 +1,10 @@
+/**
+ * @type {import('next').NextConfig}
+ */
+const nextConfig = {
+  experimental: {
+    reactOwnerStack: true,
+  },
+}
+
+module.exports = nextConfig
diff --git a/test/development/app-dir/owner-stack-react-missing-key-prop/owner-stack-react-missing-key-prop.test.ts b/test/development/app-dir/owner-stack-react-missing-key-prop/owner-stack-react-missing-key-prop.test.ts
@@ -0,0 +1,103 @@
+import { nextTestSetup } from 'e2e-utils'
+import {
+  getRedboxSource,
+  waitForAndOpenRuntimeError,
+  getStackFramesContent,
+} from 'next-test-utils'
+
+// TODO: When owner stack is enabled by default, remove the condition and only keep one test
+const isOwnerStackEnabled =
+  process.env.TEST_OWNER_STACK !== 'false' ||
+  process.env.__NEXT_EXPERIMENTAL_PPR === 'true'
+
+;(isOwnerStackEnabled ? describe : describe.skip)(
+  'app-dir - owner-stack-react-missing-key-prop',
+  () => {
+    const { next } = nextTestSetup({
+      files: __dirname,
+    })
+
+    it('should catch invalid element from on rsc component', async () => {
+      const browser = await next.browser('/rsc')
+      await waitForAndOpenRuntimeError(browser)
+
+      const stackFramesContent = await getStackFramesContent(browser)
+      const source = await getRedboxSource(browser)
+
+      if (process.env.TURBOPACK) {
+        expect(stackFramesContent).toMatchInlineSnapshot(
+          `"at Page (app/rsc/page.tsx (6:13))"`
+        )
+        expect(source).toMatchInlineSnapshot(`
+        "app/rsc/page.tsx (7:9) @ <anonymous>
+
+           5 |     <div>
+           6 |       {list.map((item, index) => (
+        >  7 |         <span>{item}</span>
+             |         ^
+           8 |       ))}
+           9 |     </div>
+          10 |   )"
+      `)
+      } else {
+        // FIXME: the owner stack method names should be `Page` instead of `map`
+        expect(stackFramesContent).toMatchInlineSnapshot(
+          `"at map (app/rsc/page.tsx (6:13))"`
+        )
+        // FIXME: the methodName should be `@ <anonymous>` instead of `@ span`
+        expect(source).toMatchInlineSnapshot(`
+        "app/rsc/page.tsx (7:10) @ span
+
+           5 |     <div>
+           6 |       {list.map((item, index) => (
+        >  7 |         <span>{item}</span>
+             |          ^
+           8 |       ))}
+           9 |     </div>
+          10 |   )"
+      `)
+      }
+    })
+
+    it('should catch invalid element from on ssr client component', async () => {
+      const browser = await next.browser('/ssr')
+      await waitForAndOpenRuntimeError(browser)
+
+      const stackFramesContent = await getStackFramesContent(browser)
+      const source = await getRedboxSource(browser)
+      if (process.env.TURBOPACK) {
+        expect(stackFramesContent).toMatchInlineSnapshot(
+          `"at Page (app/ssr/page.tsx (8:13))"`
+        )
+        expect(source).toMatchInlineSnapshot(`
+        "app/ssr/page.tsx (9:9) @ <unknown>
+
+           7 |     <div>
+           8 |       {list.map((item, index) => (
+        >  9 |         <p>{item}</p>
+             |         ^
+          10 |       ))}
+          11 |     </div>
+          12 |   )"
+      `)
+      } else {
+        // FIXME: the owner stack method names should be `Page` instead of `map`
+        expect(stackFramesContent).toMatchInlineSnapshot(
+          `"at map (app/ssr/page.tsx (8:13))"`
+        )
+        // FIXME: the methodName should be `@ <unknown>` instead of `@ p`
+        expect(source).toMatchInlineSnapshot(`
+        "app/ssr/page.tsx (9:10) @ p
+
+           7 |     <div>
+           8 |       {list.map((item, index) => (
+        >  9 |         <p>{item}</p>
+             |          ^
+          10 |       ))}
+          11 |     </div>
+          12 |   )"
+      `)
+      }
+    })
+  }
+)
diff --git a/test/development/app-dir/owner-stack-react-missing-key-prop/react-missing-key-prop.test.ts b/test/development/app-dir/owner-stack-react-missing-key-prop/react-missing-key-prop.test.ts
@@ -0,0 +1,134 @@
+import { nextTestSetup } from 'e2e-utils'
+import { getRedboxSource, waitForAndOpenRuntimeError } from 'next-test-utils'
+
+// TODO: When owner stack is enabled by default, remove the condition and only keep one test
+const isOwnerStackEnabled =
+  process.env.TEST_OWNER_STACK !== 'false' ||
+  process.env.__NEXT_EXPERIMENTAL_PPR === 'true'
+
+async function getStackFramesContent(browser) {
+  const stackFrameElements = await browser.elementsByCss(
+    '[data-nextjs-call-stack-frame]'
+  )
+  const stackFramesContent = (
+    await Promise.all(
+      stackFrameElements.map(async (frame) => {
+        const functionNameEl = await frame.$('[data-nextjs-frame-expanded]')
+        const sourceEl = await frame.$('[data-has-source]')
+        const functionName = functionNameEl
+          ? await functionNameEl.innerText()
+          : ''
+        const source = sourceEl ? await sourceEl.innerText() : ''
+
+        if (!functionName) {
+          return ''
+        }
+        return `at ${functionName} (${source})`
+      })
+    )
+  )
+    .filter(Boolean)
+    .join('\n')
+
+  return stackFramesContent
+}
+
+;(isOwnerStackEnabled ? describe.skip : describe)(
+  'app-dir - react-missing-key-prop',
+  () => {
+    const { next } = nextTestSetup({
+      files: __dirname,
+    })
+
+    let nextConfig: string = ''
+    beforeAll(async () => {
+      await next.stop()
+      await next.patchFile('next.config.js', (content: string) => {
+        nextConfig = content
+        return content.replace(
+          `reactOwnerStack: true`,
+          `reactOwnerStack: false`
+        )
+      })
+      await next.start()
+    })
+    afterAll(async () => {
+      await next.stop()
+      // Restore original next.config.js
+      await next.patchFile('next.config.js', nextConfig)
+    })
+
+    it('should catch invalid element from on rsc component', async () => {
+      const browser = await next.browser('/rsc')
+      await waitForAndOpenRuntimeError(browser)
+
+      const stackFramesContent = await getStackFramesContent(browser)
+      const source = await getRedboxSource(browser)
+
+      if (process.env.TURBOPACK) {
+        expect(stackFramesContent).toMatchInlineSnapshot(`""`)
+        expect(source).toMatchInlineSnapshot(`
+        "app/rsc/page.tsx (5:5) @ Page
+
+          3 | export default function Page() {
+          4 |   return (
+        > 5 |     <div>
+            |     ^
+          6 |       {list.map((item, index) => (
+          7 |         <span>{item}</span>
+          8 |       ))}"
+      `)
+      } else {
+        expect(stackFramesContent).toMatchInlineSnapshot(`""`)
+        // FIXME: the methodName should be `@ Page` instead of `@ div`
+        expect(source).toMatchInlineSnapshot(`
+        "app/rsc/page.tsx (5:6) @ div
+
+          3 | export default function Page() {
+          4 |   return (
+        > 5 |     <div>
+            |      ^
+          6 |       {list.map((item, index) => (
+          7 |         <span>{item}</span>
+          8 |       ))}"
+      `)
+      }
+    })
+
+    it('should catch invalid element from on ssr client component', async () => {
+      const browser = await next.browser('/ssr')
+      await waitForAndOpenRuntimeError(browser)
+
+      const stackFramesContent = await getStackFramesContent(browser)
+      const source = await getRedboxSource(browser)
+      if (process.env.TURBOPACK) {
+        expect(stackFramesContent).toMatchInlineSnapshot(`""`)
+        expect(source).toMatchInlineSnapshot(`
+        "app/ssr/page.tsx (7:5) @ Page
+
+           5 | export default function Page() {
+           6 |   return (
+        >  7 |     <div>
+             |     ^
+           8 |       {list.map((item, index) => (
+           9 |         <p>{item}</p>
+          10 |       ))}"
+      `)
+      } else {
+        expect(stackFramesContent).toMatchInlineSnapshot(`""`)
+        // FIXME: the methodName should be `@ Page` instead of `@ div`
+        expect(source).toMatchInlineSnapshot(`
+        "app/ssr/page.tsx (7:6) @ div
+
+           5 | export default function Page() {
+           6 |   return (
+        >  7 |     <div>
+             |      ^
+           8 |       {list.map((item, index) => (
+           9 |         <p>{item}</p>
+          10 |       ))}"
+      `)
+      }
+    })
+  }
+)
