Bug: Elements in ErrorBoundary are created twice, one of them becomes an orphan

[KurtGokhan]

React version: 18.2.0

Reproducible with 18.3.0-next-85de6fde5-20230328 as well.

As I observed, this is not an issue only in React-DOM. It happens with other renderers too. It can be an issue in the Reconciler or the main React package.

Issue does not seem to happen with React 17 based rendering (legacy mode?). You can test this by uncommenting the code at the end of the code example. However this can be simply because React 17 doesn't throw the error twice. In concurrent mode, error is thrown twice, so two ErrorBoundary are created. The stack traces for two errors look almost the same, except the latter error contains recoverFromConcurrentError in its call stack.

Steps To Reproduce

1. Create an ErrorBoundary class which renders some basic HTML
2. Notice how the HTML elements are created twice.

Link to code example: Visit the CodeSandbox example and observe the logs.

In the code example, I mocked the document.createElement to see that it is called twice. However, one of the created elements aren't added to the DOM (becomes orphan).

This may not be a big deal in HTML, but it can be a problem in other renderers where creating elements may have side effects. Also if the element has children, those children will also be created, potentially causing performance issues.

The current behavior

Native elements in ErrorBoundary are created twice, even though one of them are not added to the rendered tree.

The expected behavior

Elements should be created once.

[github-actions[bot]]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[KurtGokhan]

The issue is still reproducible in v19 canary. Stackblitz example here.

[eps1lon]

When React is doing a concurrent render and something throws, it will re-render one more time synchronously in case the error was caused by inconsistent data stores. There is currently a bug where we're also doing that during a synchronous render which is wasteful.

However, creating elements twice would be intended if the error happened during a concurrent render since we'd render twice. Host instance creation (e.g. document.createElement) happens during render phase. Since we only attach the final element to the DOM, previous elements should be garbage collected. There shouldn't be any problem with calling document.createElement multiple times apart from performance. But sometimes we do need to create the element ahead of time.

but it can be a problem in other renderers where creating elements may have side effects

In which renderers would that be a problem? The reconciler docs call it out explicitly that host instance creation happens during render not commit and must be side-effect free: https://github.com/facebook/react/blob/main/packages/react-reconciler/README.md#createinstancetype-props-rootcontainer-hostcontext-internalhandle

[KurtGokhan]

In which renderers would that be a problem?

I noticed this in my custom renderer for Unity. Due to the nature of the platform, host instance creation had side-effects. I fixed it in my renderer after noticing this issue.

But I can imagine this could happen in some Web Components if the author isn't aware of this maybe?

The reconciler docs call it out explicitly that host instance creation happens during render not commit and must be side-effect free

Thanks for this. I didn't know that. That particular line may not have existed in the docs back when I read it. Also this is the only case I am aware of, where a host instance is created and discarded without ever being mounted. So I was a bit surprised.

Creating elements twice would be intended if the error happened during a concurrent render since we'd render twice

Just to clarify: The error doesn't happen inside the ErrorBoundary fallback. Is it still ok that the fallback is rendered twice, not only the component which throws the error? Or maybe I should say it like this. Maybe ErrorBoundary shouldn't render in error state until synchronous render is also tried and fails.

[eps1lon]

Thanks for this. I didn't know that. That particular line may not have existed in the docs back when I read it. Also this is the only case I am aware of, where a host instance is created and discarded without ever being mounted.

There are various cases where this could happen since React can render multiple times before actually committing. A concrete case would be inside a Suspense boundary. A component may finish rendering but then a sibling suspended. When the sibling resolves, the original render may have to be discarded.

Just to clarify: The error doesn't happen inside the ErrorBoundary fallback. Is it still ok that the fallback is rendered twice, not only the component which throws the error?

We could investigate that but that wouldn't resolve the original issue, namely that host instance creation in your specific host has side-effects.

[KurtGokhan]

As long as it is intended, it is not an issue for me. It is just a tiny bit wasteful in terms of performance, but not a big priority.