fix(core): make error boundary fallback a component instead of a callback

[Josh-Cena]

Pre-flight checklist

• I have read the Contributing Guidelines on pull requests.
• If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
• If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

I realized we made a wild mistake: we should render the component instead of call it. The semantics of this prop is very unclear: we said in the docs that it's a callback and we also call it like fallback({...}), but the DefaultFallback and what's actually passed in in our internal usage are both components.

As a general principle, components should not be called directly, because (a) it may be a class and (b) it may contain hooks. This aims to make the semantics of this prop clearer by making it receive an actual component.

Test Plan

Testing is a bit hard :(

[netlify]

✅ [V2]

Name	Link
🔨 Latest commit	4dc0b7d
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/62767eeb6d74e70008703366
😎 Deploy Preview	https://deploy-preview-7368--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[github-actions]

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟠 68	🟢 100	🟢 100	🟢 100	🟢 90	Report
/docs/installation	🟠 86	🟢 99	🟢 100	🟢 100	🟢 90	Report

[github-actions]

Size Change: +25 B (0%)

Total Size: 811 kB

ℹ️ View Unchanged

Filename	Size	Change
website/.docusaurus/globalData.json	52.3 kB	0 B
website/build/assets/css/styles.********.css	105 kB	0 B
website/build/assets/js/main.********.js	615 kB	+25 B (0%)
website/build/index.html	38.8 kB	0 B

_{compressed-size-action}

[slorber]

This change is not good @Josh-Cena unfortunately

This is a bad practice in React to create components inline during the rendering, notably because the component is recreated everytime and then will always unmount/remount.

Let's use something similar to our doc example:

import React from 'react';
import ErrorBoundary from '@docusaurus/ErrorBoundary';

const SafeComponent = () => (
  <ErrorBoundary
    fallback={({error, tryAgain}) => (
      <div>
        <p>This component crashed because of error: {error.message}.</p>
        <button onClick={tryAgain}>Try Again!</button>
        <SomeStatefulComponent/>
      </div>
    )}>
    <SomeDangerousComponentThatMayThrow />
  </ErrorBoundary>
);

In this case, everything SafeComponent rerenders, the fallback component is re-created, and SomeStatefulComponent will unmount/remount and lose its state.

---

Considering there's no easy way to force users to not use inline components, I'd rather move back to the previous implementation based on a callback

Using "render callback" remains a good practice in React for some specific cases like this IMHO.

For example, React-Native has:

<FlatList
  renderItem={({ item, index, separators }) => (
    <CustomItem item={item}/>
  )}
/>

[Josh-Cena]

Previously, it was not a callback either—the default fallback is still a component (imported from @theme/Error). There's nothing really different, for people who use the default fallback.

I'm okay to make it clearer that it's a callback, not a component, but all these are (1) not fixed by using an unmemoized callback which still gets re-created (2) can be fixed by memoization, either with useCallback (with callbacks) or useMemo (with components).

[slorber]

Previously, it was not a callback either—the default fallback is still a component (imported from @theme/Error). There's nothing really different, unless people use a custom fallback.

Yes it was a callback, because it was called. The default callback signature may have been confusing because it was declared as typeof ErrorComponent (which is both a component but also matches a callback signature)

(1) not fixed by using an unmemoized callback which still gets re-created

The fact that the callback is recreated on each render is not a problem: as long as it returns the same JSX tree, the rendered components do not unmount/remount.

That's not the case with an inline component: React sees it as a different component instance every render.

(2) can be fixed by memoization, either with useCallback (with callbacks) or useMemo (with components).

useMemo is not supposed to be a 100% strong memo guarantee (it is until now, but React may decide later to evict it) and your app shouldn't rely on it to work. Creating components in useMemo should rather be avoided because there's a risk that later React will recreate the component (and thus stateful state is lost again)

I wouldn't be surprised if React later decide to evict useMemo more aggressively when using StrictMode for example

[Josh-Cena]

Right, this fix is in place because we had users who swizzled @theme/Error and used a useState in it, and that broke, because we were calling the component here (!)

I'm fine to change ?? DefaultFallback to ?? (props) => <DefaultFallback {...props} />.