[DataGrid] Uncatchable Errors thrown if row validation fails

[xadie]

• [x ] The issue is present in the latest release.
• [x ] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When row validation fails the exception thrown is uncatchable. (I guess function checkGridRowIdIsValid is responsible for throwing the error).

Expected Behavior 🤔

Exception thrown should be able to be handled from the surrounding context. I would expect that if onError callback is provided, the raised error is catched and forwarded to the callback for instance.

Steps to Reproduce 🕹

https://codesandbox.io/s/peaceful-mclean-nqcnyn?file=/src/App.tsx (v5)

• run example https://codesandbox.io/s/priceless-dewdney-9y9ko?file=/src/App.tsx (v4)
• check console output

Context 🔦

Implementing error handling for an application that uses Datagrid for data display

[dtassone]

The grid already comes with an error boundary so it should show an error overlay on errors.
There seems to be an issue with that

[oliviertassinari]

Having a look at the problem, it seems that there are multiple opportunities:

1. I believe that the main argument we have used in the past to support the idea that the data grid needs a built-in error boundary is that showing a broken UI is worth that showing nothing. We all agreed to this. React as well, so much so that they made it the default behavior:

errors that were not caught by any error boundary will result in unmounting of the whole React component tree. We debated this decision, but in our experience, it is worse to leave corrupted UI in place than to completely remove it.

https://reactjs.org/docs/error-boundaries.html

So it sounds to me that the "broken UI" argument has no foundations after all. We can remove the error boundary and still get the same outcome. @dtassone What's left to defend that we need a built-in ErrorBoundary?

2. The data grid stops the normal propagation of the error: https://codesandbox.io/s/elated-stallman-8y1cy?file=/src/App.tsx. In order to restore the default behavior, users have to rethrow it. Effectively, we prevent the developers to use the same error handling for their whole app. They have to do custom work for the data grid.

@xadie Would it help if a. there were no onError prop and you could use your own ErrorBoundary everywhere in the application? as suggested in:

In practice, most of the time you’ll want to declare an error boundary component once and use it throughout your application.

https://reactjs.org/docs/error-boundaries.html

@xadie Or as an alternative option, b. if the data grid was rethrowing so your parent ErrorBoundary can always work?

3. I can definitely notice that our built-in <ErrorBoundary> is not including all the hook logic of the data grid. If we render it higher in the React tree, then, we would lose access to the core of the logic, e.g. no apiRef.

[dtassone]

FYI @oliviertassinari the datagrid is NOT swallowing errors as you can see in https://material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-error-handling--throw-exception

[oliviertassinari]

NOT swallowing errors

@dtassone I didn't use this terminology but it depends on which dimension we look at. What I was considering is for developers to use the React error boundary pattern in their application. They can't use their own error boundary unless they manually rethrow, or we move with option a. or b.

I have provided proof in:

The data grid stops the normal propagation of the error: https://codesandbox.io/s/elated-stallman-8y1cy?file=/src/App.tsx.

I think that the most important aspect is why should we handle errors. How do we justify that one component needs a built-in error boundary? I personally fail to envision the value.

---

At this point, I think that we should wait to hear more feedback from users like @xadie to know what's the most frequent use case/pain we can solve regarding error handling.

[m4theushw]

This issue doesn't appear to be ready to be worked on. IHMO we should remove our ErrorBoundary and let users use their own. I think it's better to not have one than to have one that doesn't catch all possible errors, e.g. the one related by the OP. From my experience, most of the errors in an application come from promises and these React doesn't catch. Therefore, only one root ErrorBoundary is enough.

[oliviertassinari]

@m4theushw I also feel that we don't have enough information around how developers handle error handling. I'm personally in the same boat as you are, and it's what React documents.

But it doesn't mean that most devs are. For instance, maybe it's better to only crash the data grid and not the rest of the page?

At least we don't document onError, so we could remove the prop if needed during a minor version change.

On the other hand, a prop to display an error inside the data grid seems valuable, e.g. network fail.

[m4theushw]

At least we don't document onError, so we could remove the prop if needed during a minor version change.

It's in https://material-ui.com/api/data-grid/.

On the other hand, a prop to display an error inside the data grid seems valuable, e.g. network fail.

👍

[oliviertassinari]

Last summer, I didn't port https://master--material-ui-x.netlify.app/storybook/?path=/story/1-docs-13-error-handling--page into the docs because the feature felt too premature.

Handling error on the UI and in the grid, is critical to avoid breaking the whole application, or even displaying corrupted data. Therefore, XGrid is wrapped with an error boundary internally. So if the component throws an unhandled exception anywhere in the tree, the exception will get handled by the error boundary, and a nice error message overlay will be displayed.

As we have seen "displaying corrupted data" is not a valid argument, the real argument seems around not "breaking the whole application", only the data grid. The behavior comes at the cost of breaking the natural flow React error boundary. You can only get one of the two.

Maybe we should:

• Document the error prop
• make onError private/remove it
• optimize for the principle of least surprise? Meaning developers don't expect any component to come with an error boundary, we would remove it. Most of the time, developers handle gracefully the common errors anyway.
  If there is a bug that crashes the page, so be it, end-users are used to reload when this happens. Developers could still add an error boundary if they find it relevant, we won't force it on them.

[dtassone]

Errors can come from many different sources not only ajax. It's wrong to assume that. It could be a corrupt/bad data in the DB that cause the grid to crash due to a rendercell for example...
The goal is to create a self contained component that handle its errors in a nice way. As mentioned errors are not swallowed they are thrown. So one can easily handle it differently if it suits.
Async errors should be handled by calling showError of apiRef.

[oliviertassinari]

errors are not swallowed they are thrown. So one can easily handle it differently if it suits.

@dtassone This is not accurate, see https://codesandbox.io/s/elated-stallman-8y1cy?file=/src/App.tsx. Developers have to rethrow the error in order for their own error boundary to catch it. If you don't want the error boundary of the data grid, it's relatively painful to override.

[m4theushw]

The action to close this issue is to remove the error boundary and let the errors propagate.

diff --git a/packages/grid/x-data-grid/src/DataGrid/DataGrid.tsx b/packages/grid/x-data-grid/src/DataGrid/DataGrid.tsx
index a9ed12e71..41ffa8456 100644
--- a/packages/grid/x-data-grid/src/DataGrid/DataGrid.tsx
+++ b/packages/grid/x-data-grid/src/DataGrid/DataGrid.tsx
@@ -26,14 +26,12 @@ const DataGridRaw = React.forwardRef(function DataGrid<R extends GridValidRowMod
   return (
     <GridContextProvider apiRef={apiRef} props={props}>
       <GridRoot className={props.className} style={props.style} sx={props.sx} ref={ref}>
-        <GridErrorHandler>
-          <GridHeaderPlaceholder />
-          <GridBody
-            ColumnHeadersComponent={DataGridColumnHeaders}
-            VirtualScrollerComponent={DataGridVirtualScroller}
-          />
-          <GridFooterPlaceholder />
-        </GridErrorHandler>
+        <GridHeaderPlaceholder />
+        <GridBody
+          ColumnHeadersComponent={DataGridColumnHeaders}
+          VirtualScrollerComponent={DataGridVirtualScroller}
+        />
+        <GridFooterPlaceholder />
       </GridRoot>
     </GridContextProvider>
   );

[m4theushw]

The example is from v4, I updated it for v5. The initial behavior of swallowing errors from row validation doesn't appear anymore in v5. The error boundary now is only triggered if, for example, a cell or row throws an error. We can still remove the error boundary.