Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Error Boundaries for Function Components #126

Closed
wants to merge 4 commits into from
Closed

RFC: Error Boundaries for Function Components #126

wants to merge 4 commits into from

Conversation

GasimGasimzada
Copy link

@GasimGasimzada GasimGasimzada commented Oct 26, 2019

In this RFC, I am talking about adding error boundaries to functional components:

const MyErrorBoundary = React.errorBoundary((props, error, stack) => {
    // This code segment is equivalent to `componentDidCatch`
    useEffect(() => {
        if (error !== null) {
            logErrorToBackend(error, stack);
        }
    }, [error]); // dependency is important here to not sent the information on every render

    // No need to derive state from error anymore. Just use the error value to do whatever you want
    if (error) {
        return <ErrorBox error={error} />
    }

    return props.children;
});

This will allow developers to use function components for all features of React. Currently, Error Boundaries is the only reason to create a class component in React. I tried to make the API similar to React.memo and React.forwardRef.

I am open for suggestions to the API (e.g send the errorInfo object or send error and stack separately).

@dantman
Copy link

dantman commented Oct 26, 2019

I have a few issues with the error boundary API in this format.

error isn't really a permanent state that should be passed in, it's an internal effect that changes state once. The circumstances that lead up to the error can be recovered from out of band.

For instance I have hot-reloading enabled and I put something along these lines in development to add a Retry button to my error boundary so I can test the fixed code without reloading the whole page.

let retry;
if (process.env.NODE_ENV === 'development') {
  retry = (
    <Button onClick={() => this.setState({ hasError: false })}>
      Retry
    </Button>
  );
}

And I believe that situations where you want to remove that error/hasError state are going to become more common.

Suspense's data-fetching functionality has already made it into an experimental release. Suspense encourages you to do data loading as part of render instead of effects, i.e. you set state to a promise and Suspense handles waiting for that promise to resolve.

I believe this is going to result in error boundaries starting to get errors related to data fetching that would normally be handled inside of an effect. As a result, error boundaries are going to be getting a lot of recoverable errors.

I think an API something like this would be better suited to React's future.

const MyErrorBoundary = ({children}) => {
  const [error, setError] = useErrorBoundary((error, info) => {
    // Commit phase effect like componentDidCatch
    logErrorToBackend(error, info);
  });
  const retry = useCallback(() => {
    setError(null);
  }, [setError]);

  if (error) {
    if (isNetworkError(error)) {
      return (
        <NetworkErrorState error={error} onRetry={retry} />
      );
    } else {
      throw error;
    }
  }

  return children;
});

@GasimGasimzada
Copy link
Author

GasimGasimzada commented Oct 26, 2019

The reason why I suggested with the HOC route to set up Error Boundaries was that HOC just passes the value to the component. It doesn't perform any other action on the component. What you do with the value is up to you. You can still store error value in state if you like:

const MyErrorBoundary = React.errorBoundary((props. error, info) => {
  const [hasError, setHasError] = useState(false);
  useEffect(() => {
    setHasError(error !== null);
  }, [error]);

  // ... rest of the code
});

I know it is verbose but I will get back to it later.

When using hooks to "activate" error boundary, there are couple of things that concern for me. Firstly, how is the error boundary going to be identified from React's point of view. Currently, Reconciler checks whether the component has getDerivedStateFromError or componentDidCatch functions. When you call this hook, there needs to be a way to store error boundaries per Fiber. We can add a parameter to the Fiber object when dispatching useErrorBoundary hook but is that the desirable behavior from library maintenance point of view? I can't answer that but I wouldn't want every Fiber object having an "errorBoundary" parameter when usage. This is my personal opinion of course and React team might not see this as a problem.

Secondly, what if you have multiple useErrorBoundary hooks? Are you going to activate all of them? You might say that nobody will use more than one error boundary hook per component. It might even be logical to throw an error if more than one error boundary is present per component. However, to me personally, the only acceptable solution is for Reconciler to run all the available hooks. Or it is just another rule to not run the component in separation. If the above concern is resolved via a single parameter, this might be a feasible option. However, I still think that this needs to be restricted from API perspective. You shouldn't be able to activate more than one error boundary per component.

Thirdly, the API that you provided for Error Boundary is a little bit convoluted. You are returning a local state value from the hook while the hook name does not reflect that. Every time I look at the code above, I think that the actual error is being cleared instead of the local value. Secondly, the argument of the component is confusing. The callback function to log errors has nothing to do with the returned value. My guess is that it is dependent on the incoming error, not the local state. So, what happens when I call setError(somethingElse)? Will it get called again or will it be ignored because it does not rely on local state? Separating these concerns will make the code more readable and understandable.

As I have mentioned in my RFC, I am open to adding a specific hook to simplify error boundary logic. Based on your suggestions and my HOC, I suggest changing the solution to something like this:

const MyErrorBoundary = React.errorBoundary((props, errorInfo) => {
   const [error, setError] = useDerivedStateFromError(errorInfo);

   useEffectForError((error, stack) => {
     logErrorToBackend(error, stack);
   }, errorInfo);

  // ... rest of the code   
});

This way, you can create multiple effects and states for errors while still having a single identifier for the error boundary (the HOC). I know it is a lot of things to import but it is clearer to understand. Additionally, the hooks that I used are just helpers and they can be created in user space by using useState and useEffect hooks.

Note:

errorInfo object stores both error object and the stack. This is similar to the error object that is used by React's Reconciler: (https://github.com/facebook/react/blob/f6b8d31a76cbbcbbeb2f1d59074dfe72e0c82806/packages/react-reconciler/src/ReactFiberThrow.js#L93)

What to use for error object is still a question for me.

- Add "Other Concerns" w/ Alternate approach
- Add a common use-case example in "Basic example"
@itsMapleLeaf
Copy link

itsMapleLeaf commented Oct 28, 2019

How would one use errorBoundary and forwardRef on the same component? Sorry if that's noted somewhere and I've missed it

Either way, here's what I've thought about an alternate design (which is actually possible in userland now, but this would still allow us to have 100% function components)

function Example() {
  const [error, setError] = useState()

  const handleError = (error, info) => {
    errorLogger.report(error, info)
    setError(error)
  }

  if (error) {
    return (
      <>
        <p>Oops! An error occurred</p>
        <button onClick={() => setError(undefined)}>Restart app</button>
      </>
    )
  }

  return (
    <ErrorBoundary onCatch={handleError}>
      <App />
    </ErrorBoundary>
  )
}

@j-f1
Copy link

j-f1 commented Oct 28, 2019

How about this alternative API?

function Example() {
  const [error, setError] = useState()
  useEffect(() => {
    if (error) {
      errorLogger.report(error.error, error.stack)
    }
  }, [error])

  const ErrorBoundary = useErrorBoundary((error, stack) => {
    if (error.isSomethingWeWantToHandleHere()) {
      // handle the error by calling a state setter
      setError({ error, stack })
    } else {
      // or re-throw the error so it can be handled by a parent ErrorBoundary:
      throw error
    }
  })

  return error ? (
    <div>
      <p>Oops! An error occurred</p>
      <button onClick={() => setError()}>Restart app</button>
    </div>
  ) : (
    <ErrorBoundary>
      Children are rendered directly if no error occurs
    </ErrorBoundary>
  )
}
Example API provided by an error reporting service:
function useSentryBoundary({ predicate, ...opts }) {
  const [error, setError] = useState()
  const key = useContext(SentryAPIKeyContext)
  useEffect(() => {
    if (error) {
      callSentryAPI({ key, opts, error })
    }
  }, [error])

  const ErrorBoundary = useErrorBoundary((error, stack) => {
    if (!predicate || predicate(error)) {
      setError({ error, stack })
    } else {
      throw error
    }
  })
  return [ErrorBoundary, useCallback(() => setError(), [])]
}

function Example() {
  const [ErrorBoundary, clearError] = useSentryBoundary({
    level: 'error',
    predicate: err => err.isSomethingWeWantToHandleHere()
  })
  return error ? (
    <div>
      <p>Oops! An error occurred</p>
      <button onClick={clearError}>Restart app</button>
    </div>
  ) : (
    <ErrorBoundary>
      Children are rendered directly if no error occurs
    </ErrorBoundary>
  )
}

@GasimGasimzada
Copy link
Author

GasimGasimzada commented Oct 28, 2019

@kingdaro ForwardRef function can already be combined with memo HOC and it needs to have a specific order. Here is warning that you get when using memo inside forwardRef

'forwardRef requires a render function but received a memo component. Instead of forwardRef(memo(...)), use memo(forwardRef(...)).

Link

If we create an error boundary HOC that returns a new, specific error boundary type, we can just use them together. I am not sure of implementation details (still investigating the code to understand how memo and forwardRef are not clashing with each other).


@j-f1 @kingdaro There is one issue that I have with this (this is how I view it; so, I am willing to get challenged on this point of view). When using ErrorBoundary like a component, the Example component loses its identification as the error boundary. It is like a "Container of Error Boundary" (similar to what we used to do pre-hooks to separate data logic from view logic -- e.g in Redux). On the other hand, in current Error Boundary implementation the components are Error Boundaries on their own. I can't answer whether this kind of change in design is okay.

In my last update to the RFC, I have added an alternate approach to using error boundaries as hooks. @j-f1 Based on my latest update, if using hooks to identify error boundaries is an okay approach (I have explained it in the RFC regarding one possible way to retrieve the error for the error boundary), I like this design a lot because state is being set using normal hooks:

useErrorBoundary((error, stack) => {
  if (!predicate || predicate(error)) {
    setError({ error, stack })
  } else {
    throw error
  }
})

@j-f1
Copy link

j-f1 commented Oct 28, 2019

I feel like the approach of having an actual component in the tree is valuable because

When using ErrorBoundary like a component, the Example component loses its identification as the error boundary

I don’t think this is a negative. Instead of having arbitrary components be error boundaries, you can find the specific ErrorBoundary component in the tree of rendered components.

@j-f1
Copy link

j-f1 commented Nov 7, 2019

Here’s a working demo of the component-returning hook API: https://observablehq.com/d/b21a867e80da0a00.

Since this is possible and effective in userland, maybe there doesn’t need to be an official built-in API?

Implementation:

function useErrorBoundary(handler) {
  // use a ref instead of recreating the component on every render
  const componentRef = useRef();
  // ref the handler so the latest one is always called,
  // with access to the current props and state.
  const handlerRef = useRef();
  handlerRef.current = handler;
  if (!componentRef.current) {
    componentRef.current = class ErrorBoundary extends React.Component {
      componentDidCatch(...args) {
        // pass the args directly to the error handler
        handlerRef.current(...args);
      }
      // required to suppress a warning from React
      static getDerivedStateFromError(err) {
        return { err };
      }
      render() {
        return this.props.children;
      }
    };
  }
  return componentRef.current;
}

@dantman
Copy link

dantman commented Oct 18, 2020

I was thinking about error boundaries and just thinking about writing an RFC for the idea I thought of, and completely forgot I had commented on a previous error boundary proposal.

What do you all think of this API?

function ErrorBox() {
  const [error, retry] = useError();
  useEffect(() => {
    if (error) {
      errorLogger.report(error.error, error.stack);
    }
  }, [error]);

  return (
    <div>
      <p>Oops! An error occurred</p>
      <button onClick={() => retry()}>Restart app</button>
    </div>
  );
}

function Example() {
  return (
    <ErrorBoundary
      is={(error) => error.isSomethingWeWantToHandleHere()}
      fallback={<ErrorBox />}
    >
      Children are rendered directly if no error occurs
    </ErrorBoundary>
  );
}

It's heavily inspired by the <Suspense> API and I think it fits in most with the direction that React is taking.

I'm also considering the possibility of an additional method of use:

<ErrorBoundary is={isNetworkError}>
  <App />
</ErrorBoundary>

The theory being for some errors like network errors the error boundary could still render the application instead of a fallback and somewhere in the application a useError could display an error message like a network error banner added by the layout component. Though I'm still contemplating how/whether this would actually work.

Edit: If we like this API maybe we could modify react-error-boundary to support it as an experiment till we get a class-less built-in API.

@gaearon
Copy link
Member

gaearon commented Aug 18, 2021

A looong time ago when we discussed this, there was a suspicion that error handling might tie into a broader suite of features related to control flow. See #11 (comment) for more context.

In particular, there was a question whether we could use generators for error boundaries.

function *Foo({ children }) {
  let [showError, updateError] = useState(false);
  if (showError) {
    return <Error retry={() => updateError(false)} />;
  }
  try {
    return yield children;
  } catch (x) {
    updateError(true);
  }
}

We haven't talked a lot about that idea though since it's a relatively rarely used feature (usually you write a reusable boundary once and then keep reusing it).

There were also, of course, more "normal" proposals. Such as something similar to the one in this RFC. However, it's important that error is a value that's "passed in" rather than held by React since you need to have control over when to reset it.

let ErrorBoundary = React.catch((props, newError) => {
  let [error, setError] = useState(null);
  if (newError) {
    // getDerivedStateFromError
    setError(newError);
  }

  useEffect(() => {
    // componentDidCatch
    logError(error);
  }, [error]);

  if (error) {
    return (
      <div>
        Error! {error.message}
        <button onClick={() => setError(null)}>Retry</button>
      </div>
    );
  }
  return props.children;
});

There was also a proposal that was more like one of the comments here.

function ExampleErrorBoundary({ children }) {
  const [hasError, setHasError] = useState(false);

  useErrorBoundary(
    // Callback to be used for updating state
    (error) => {
      // Update state to re-render with fallback UI.
      setHasError(true);
    },

    // Optional commit phase callback to be used for e.g. logging
    (error, info) => {
      logComponentStackToMyService(info.componentStack);
    }
  );

  if (hasError) {
    return <h1>Something went wrong.</h1>;
  }

  return children; 
}

We haven't dived into the options or done the research on this, and we don't expect to come back to this topic until later.

@acdlite acdlite deleted the branch reactjs:master September 1, 2021 15:17
@acdlite acdlite closed this Sep 1, 2021
@acdlite
Copy link
Member

acdlite commented Sep 1, 2021

This accidentally got closed when I renamed master -> main... Sorry about that.

Though I think we probably will keep it closed since it's not an active area of research, for the reasons @gaearon described above. Will consider re-opening when we return to the topic.

@symbiont-matthew-pirocchi

@acdlite If this is not an active area of research, should the hooks FAQ be updated? It makes it sound like this feature is imminent:

Our goal is for Hooks to cover all use cases for classes as soon as possible. There are no Hook equivalents to the uncommon getSnapshotBeforeUpdate, getDerivedStateFromError and componentDidCatch lifecycles yet, but we plan to add them soon.

FWIW, I would still very much love to see this feature. The error boundary component is the only non-functional component in the project I'm working on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants