feat(react): prevent recursive exposing fallback when fallback throw …

…error (#1409) 🚧 This PR could make a huge difference, so please help us make sure this change gets a very thorough review. 🚧 ```tsx const Throw = { Error: ({ message, after = 0, children }: PropsWithChildren<{ message: string; after?: number }>) => { const [isNeedThrow, setIsNeedThrow] = useState(after === 0) if (isNeedThrow) { throw new Error(message) } useTimeout(() => setIsNeedThrow(true), after) return <>{children}</> } } const Example = () => ( <ErrorBoundary fallback={() => <>This is expected</>}> <ErrorBoundary fallback={() => ( <Throw.Error message={ERROR_MESSAGE} after={100}> ErrorBoundary's fallback before error </Throw.Error> )} > <Throw.Error message={ERROR_MESSAGE} after={100}> ErrorBoundary's children before error </Throw.Error> </ErrorBoundary> </ErrorBoundary> ) ``` ## Problem: ErrorBoundary's fallback can't be treated by parent ErrorBoundary  Thrown Error in fallback will be caught by ErrorBoundary self and then expose fallback recursively 🥲 1. ErrorBoundary's children before error 2. ErrorBoundary's fallback before error 3. ErrorBoundary's fallback before error 4. ErrorBoundary's fallback before error 5. ... expose fallback self recursively ... ## Solution: When we meet thrown error in fallback of ErrorBoundary wrap it as InternalFallbackError, re-throw InternalFallbackError.fallbackError, if it is InternalFallbackError Thrown Error in fallback will be caught by parent ErrorBoundary 👍 1. ErrorBoundary's children before error 2. ErrorBoundary's fallback before error 3. This is expected ## PR Checklist - [x] I did below actions if need 1. I read the [Contributing Guide](https://github.com/toss/suspensive/blob/main/CONTRIBUTING.md) 5. I added documents and tests. --------- Co-authored-by: Lee HyunJae (whale) <[email protected]> Co-authored-by: lucas0530 <[email protected]> Co-authored-by: HYUNGU KANG <[email protected]> Co-authored-by: Brian Vaughn <[email protected]>
toss · Feb 1, 2025 · 124238c · 124238c
1 parent 59a809d
commit 124238c
Show file tree

Hide file tree

Showing 8 changed files with 176 additions and 18 deletions.
diff --git a/.changeset/pretty-brooms-float.md b/.changeset/pretty-brooms-float.md
@@ -0,0 +1,5 @@
+---
+"@suspensive/react": minor
+---
+
+feat(react): prevent recursive expose fallback when fallback throw error
diff --git a/examples/visualization/package.json b/examples/visualization/package.json
@@ -23,6 +23,7 @@
     "next": "catalog:",
     "react": "catalog:react19",
     "react-dom": "catalog:react19",
+    "react-error-boundary": "^5.0.0",
     "sharp": "catalog:",
     "zod": "^3.24.1"
   },

diff --git a/examples/visualization/src/app/layout.tsx b/examples/visualization/src/app/layout.tsx
@@ -31,6 +31,9 @@ export default function RootLayout({ children }: { children: React.ReactNode })
                 <li>
                   <Link href="/react/ErrorBoundary/shouldCatch">{`<ErrorBoundary/>`} shouldCatch prop</Link>
                 </li>
+                <li>
+                  <Link href="/react/ErrorBoundary/ErrorInFallback">{`<ErrorBoundary/>`}'s fallback Error</Link>
+                </li>
                 <li>
                   <Link href="/react/ErrorBoundary/shouldCatch/renderPhase">
                     {`<ErrorBoundary/>`} shouldCatch prop render phase

diff --git a/examples/visualization/src/app/react/ErrorBoundary/ErrorInFallback/page.tsx b/examples/visualization/src/app/react/ErrorBoundary/ErrorInFallback/page.tsx
@@ -0,0 +1,71 @@
+'use client'
+
+import { ErrorBoundary as SuspensiveErrorBoundary } from '@suspensive/react'
+import { type PropsWithChildren, useCallback, useEffect, useRef, useState } from 'react'
+import { ErrorBoundary as ReactErrorBoundary } from 'react-error-boundary'
+import { Area } from '~/components/uis'
+
+export const useTimeout = (fn: () => void, ms: number) => {
+  const fnRef = useRef(fn)
+  fnRef.current = fn
+  const fnPreserved = useCallback(() => fnRef.current(), [])
+  useEffect(() => {
+    const id = setTimeout(fnPreserved, ms)
+    return () => clearTimeout(id)
+  }, [fnPreserved, ms])
+}
+
+export const Throw = {
+  Error: ({ message, after = 0, children }: PropsWithChildren<{ message: string; after?: number }>) => {
+    const [isNeedThrow, setIsNeedThrow] = useState(after === 0)
+    if (isNeedThrow) {
+      throw new Error(message)
+    }
+    useTimeout(() => setIsNeedThrow(true), after)
+    return <>{children}</>
+  },
+}
+
+export default function Page() {
+  return (
+    <div>
+      <Area title="@suspensive/react">
+        <SuspensiveErrorBoundary fallback={() => <>This is expected</>}>
+          <SuspensiveErrorBoundary
+            fallback={() => {
+              console.log("@suspensive/react's ErrorBoundary fallback")
+              return (
+                <Throw.Error message={'error message in fallback'} after={1000}>
+                  SuspensiveErrorBoundary's fallback before error
+                </Throw.Error>
+              )
+            }}
+          >
+            <Throw.Error message={'error message in children'} after={1000}>
+              SuspensiveErrorBoundary's children before error
+            </Throw.Error>
+          </SuspensiveErrorBoundary>
+        </SuspensiveErrorBoundary>
+      </Area>
+
+      <Area title="react-error-boundary">
+        <ReactErrorBoundary fallbackRender={() => <>This is expected</>}>
+          <ReactErrorBoundary
+            fallbackRender={() => {
+              console.log("react-error-boundary's ErrorBoundary fallback")
+              return (
+                <Throw.Error message={'error message in fallback'} after={1000}>
+                  ReactErrorBoundary's fallback before error
+                </Throw.Error>
+              )
+            }}
+          >
+            <Throw.Error message={'error message in children'} after={1000}>
+              ReactErrorBoundary's children before error
+            </Throw.Error>
+          </ReactErrorBoundary>
+        </ReactErrorBoundary>
+      </Area>
+    </div>
+  )
+}
diff --git a/packages/react/package.json b/packages/react/package.json
@@ -53,7 +53,8 @@
     "@suspensive/tsconfig": "workspace:*",
     "@suspensive/tsup": "workspace:*",
     "@types/react": "catalog:react19",
-    "react": "catalog:react19"
+    "react": "catalog:react19",
+    "react-error-boundary": "^5.0.0"
   },
   "peerDependencies": {
     "react": "^18 || ^19"

diff --git a/packages/react/src/ErrorBoundary.spec.tsx b/packages/react/src/ErrorBoundary.spec.tsx
@@ -2,6 +2,7 @@ import { act, render, screen, waitFor } from '@testing-library/react'
 import { userEvent } from '@testing-library/user-event'
 import ms from 'ms'
 import { type ComponentRef, createElement, createRef } from 'react'
+import { ErrorBoundary as ReactErrorBoundary } from 'react-error-boundary'
 import {
   ErrorBoundary,
   type ErrorBoundaryFallbackProps,
@@ -313,6 +314,54 @@ describe('<ErrorBoundary/>', () => {
       await waitFor(() => expect(screen.queryByText(errorText)).toBeInTheDocument())
     }
   )
+
+  it('should re-throw error in fallback', async () => {
+    render(
+      <ErrorBoundary fallback={() => <>This is expected</>}>
+        <ErrorBoundary
+          fallback={() => (
+            <Throw.Error message={ERROR_MESSAGE} after={100}>
+              ErrorBoundary's fallback before error
+            </Throw.Error>
+          )}
+        >
+          <Throw.Error message={ERROR_MESSAGE} after={100}>
+            ErrorBoundary's children before error
+          </Throw.Error>
+        </ErrorBoundary>
+      </ErrorBoundary>
+    )
+
+    expect(screen.queryByText("ErrorBoundary's children before error")).toBeInTheDocument()
+    await waitFor(() => expect(screen.queryByText("ErrorBoundary's fallback before error")).toBeInTheDocument())
+    await waitFor(() => expect(screen.queryByText('This is expected')).toBeInTheDocument())
+  })
+  it('should not re-throw error in fallback (react-error-boundary)', async () => {
+    render(
+      <ReactErrorBoundary fallbackRender={() => <>This is expected</>}>
+        <ReactErrorBoundary
+          fallbackRender={() => (
+            <Throw.Error message={ERROR_MESSAGE} after={100}>
+              ErrorBoundary(react-error-boundary)'s fallback before error
+            </Throw.Error>
+          )}
+        >
+          <Throw.Error message={ERROR_MESSAGE} after={100}>
+            ErrorBoundary(react-error-boundary)'s children before error
+          </Throw.Error>
+        </ReactErrorBoundary>
+      </ReactErrorBoundary>
+    )
+
+    expect(screen.queryByText("ErrorBoundary(react-error-boundary)'s children before error")).toBeInTheDocument()
+    await waitFor(() =>
+      expect(screen.queryByText("ErrorBoundary(react-error-boundary)'s fallback before error")).toBeInTheDocument()
+    )
+    await waitFor(() => expect(screen.queryByText('This is expected')).not.toBeInTheDocument())
+    await waitFor(() =>
+      expect(screen.queryByText("ErrorBoundary(react-error-boundary)'s fallback before error")).toBeInTheDocument()
+    )
+  })
 })
 
 describe('<ErrorBoundary.Consumer/>', () => {

diff --git a/packages/react/src/ErrorBoundary.tsx b/packages/react/src/ErrorBoundary.tsx
@@ -110,6 +110,9 @@ class BaseErrorBoundary extends Component<ErrorBoundaryProps, ErrorBoundaryState
       if (error instanceof SuspensiveError) {
         throw error
       }
+      if (error instanceof ErrorInFallback) {
+        throw error.originalError
+      }
       const isCatch = Array.isArray(shouldCatch)
         ? shouldCatch.some((shouldCatch) => checkErrorBoundary(shouldCatch, error))
         : checkErrorBoundary(shouldCatch, error)
@@ -124,12 +127,12 @@ class BaseErrorBoundary extends Component<ErrorBoundaryProps, ErrorBoundaryState
         throw error
       }
 
-      if (typeof fallback === 'function') {
-        const FallbackComponent = fallback
-        childrenOrFallback = <FallbackComponent error={error} reset={this.reset} />
-      } else {
-        childrenOrFallback = fallback
-      }
+      const Fallback = fallback
+      childrenOrFallback = (
+        <FallbackBoundary>
+          {typeof Fallback === 'function' ? <Fallback error={error} reset={this.reset} /> : Fallback}
+        </FallbackBoundary>
+      )
     }
 
     return (
@@ -138,6 +141,22 @@ class BaseErrorBoundary extends Component<ErrorBoundaryProps, ErrorBoundaryState
   }
 }
 
+class ErrorInFallback extends Error {
+  originalError: Error
+  constructor(originalError: Error) {
+    super()
+    this.originalError = originalError
+  }
+}
+class FallbackBoundary extends Component<{ children: ReactNode }> {
+  componentDidCatch(originalError: Error) {
+    throw originalError instanceof SuspensiveError ? originalError : new ErrorInFallback(originalError)
+  }
+  render() {
+    return this.props.children
+  }
+}
+
 /**
  * This component provides a simple and reusable wrapper that you can use to wrap around your components. Any rendering errors in your components hierarchy can then be gracefully handled.
  * @see {@link https://suspensive.org/docs/react/ErrorBoundary Suspensive Docs}

diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml