feat(core): Add React ErrorBoundary component + theme default boundaries

[spyke01]

Motivation

Close #2889

Added a configurable error boundary at the App component to wrap it and contain most errors without the need to add the error boundary at the clientEntry and serverEntry level.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Yarn start and then create a breaking issue in lower level component to test for ErrorBoundary containing the error. For example, we take advantage of the known error (#5555) to test the error boundary.

Note: this is not intended to be a fix for that issue. We should wrap the live codeblock in this error boundary once we merge this.

Before:

After:

[docusaurus-bot]

✔️ [V2]

🔨 Explore the source changes: de5c9b3

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6183f0c5bdfabd0008639cc0

😎 Browse the preview: https://deploy-preview-3104--docusaurus-2.netlify.app

[slorber]

Thanks, will review it soon.

Important note: if rendering fails on the server, we need to fail fast, and not catch the rendering errors. We don't want to output static html files with errors in it without noticing :)

Also, we should apply one error boundary at the app level for maximum security, but I think we should also integrate this in our classic theme. As it's generally the "inner content" of a page that crashes, I think our theme Layout component should add another error boundary around its children props, so that if the inner content crashes, user can still see the navbar and navigate away from this error.

As we encourage users to implement their own themes, we could provide a helper comp. Here's how I see it used in theme layout:

export default function MyThemeLayout({ children }) {
  return (
    <div>
      <Navbar />
      <main>
        <DocusaurusErrorBoundary
          renderError={({ error, tryAgain }) => {
            return (
              <div>
                <h1>Hey, the app crashed: {error.message}</h1>
                <button onClick={() => tryAgain()}>Try again (attempt to re-render)</button>
              </div>
            );
          }}
        >
          {children}
        </DocusaurusErrorBoundary>
      </main>
    </div>
  );
}

This comp will only catch client-side errors, and give a hook to attempt a new re-render (sometimes it can fix the issue)

What do you think?

[spyke01]

I like that execution for the theme layout error boundary. It also lets us reuse that same design in the Docusaurus theme Layout component.

For resolving the server rendering issue, does the server render sent the mode as production? If so I can create another flat gon the boundary that disables the catching on production at the App level.

[slorber]

There is ExecutionEnvironment.canUseDOM. If it can't use dom, we should just not call setState({error}) and the error will bubble up and crash the build, as we want.

This feature needs 2 components:

• DocusaurusErrorBoundary, that we'll allow theme to implement (in client/exports folder)
• Error, the "default" unthemed error screen, in client/theme-fallback)

The "Error" component would render a basic error screen with rough styles, but will let the themes like the classic theme override it to provide a better-looking error screen.

[spyke01]

Thanks for the feedback, I will work on these changes and push them back for review after testing.

[spyke01]

I've pushed commits to try and address these issues. My typescript is rusty so I wasn't able to specify a function typing on therederError or tryAgain functions.

For DocusaurusErrorBoundary I tried to make it so that if renderError wasn't passed it would fallback to the theme and use the tryAgain param to handle the try again button. That way whatever section is being wrapped could have it's own try again passed in.

Let me know your thoughts or any tweaks you would recommend.

[slorber]

Thanks, that looks nice.

Changes needed:

• DocusaurusErrorBoundary should not receive tryAgain as props. Instead, it should provides it to renderError)
• TS: tryAgain?: () => void;
• We need a beautiful themed way to display errors in the classic theme (theme/Error/index.tsx in theme classic to override the theme-fallback ugly default Error
• Documentation doc DocusaurusErrorBoundary + Error

[slorber]

Hi @spyke01

Do you plan to finish this PR? Or I can finish it for you?

[spyke01]

Thanks for checking, I haven't been able to set aside the time to do this. If you are willing to take this on it would be greatly appreciated.

…

On Thu, Aug 20, 2020 at 11:05 AM Sébastien Lorber ***@***.***> wrote: Hi @spyke01 <https://github.com/spyke01> Do you plan to finish this PR? Or I can finish it for you? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3104 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIBLQHPMA2KL36YDRLTJKLSBVCQLANCNFSM4PF3LIAQ> .

[slorber]

Thanks for letting me know, we'll figure out a way to get this code merged

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟢 Performance	95
🟢 Accessibility	98
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	95

Lighthouse ran on https://deploy-preview-3104--docusaurus-2.netlify.app/

[Josh-Cena]

@slorber I've resurrected this PR. Do things look good?

(Afaik componentDidCatch would only fire in the browser, so server build errors should always be reported, but I don't know how to test that)

[Josh-Cena]

/ping @slorber Ready for review

[slorber]

Thanks, made some little changes, LGTM 👍

• Test page: https://deploy-preview-3104--docusaurus-2.netlify.app/tests/pages/error-boundary-tests
• Doc: https://deploy-preview-3104--docusaurus-2.netlify.app/docs/next/docusaurus-core/#errorboundary
• Root error boundary should not assume that rendering the layout is safe to render (users can swizzle it, and later provide buggy custom navbar components...) => need an extra error boundary 🤪

[Josh-Cena]

LGTM

There's a slight inconsistency between theme-fallback/Error and theme-classic/ErrorPageContent (center vs left aligned) but it looks fine

[slorber]

There's a slight inconsistency between theme-fallback/Error and theme-classic/ErrorPageContent (center vs left aligned) but it looks fine

Yes, that doesn't seem like a big deal to me, the goal is not necessarily that those look the same, as we should hope that the inner boundary will catch most errors. Layout errors are expected but if you have one you likely have an important issue to fix 😆