mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.06k stars 1.25k forks source link

[DataGrid] Consider bringing back the error overlay #11399

Open Janpot opened 9 months ago

Janpot commented 9 months ago

Summary šŸ’”

Mostly useful when doing serverside pagination/filtering. The main suggestion I see on the issue tracker seems to be to create an overlay for the whole grid in case a data loading error happens. The specific use-case where this setup is problematic is the one where the usage of a specific filter results in an error. One could get the grid back to the working state by simply removing the filter. but since the grid has been fully overlaid, one can't access the filter controls anymore. An error overlay that targets specifically the rows area doesn't have this problem.

Illustration of the problem: https://stackblitz.com/edit/react-xffwnt?file=Demo.tsx in this demo, After adding a filter to this id column, the whole grid disappears behind the error overlay.

In Toolpad at the moment we've hacked around this problem by hijacking the NoRowsOverlay slot, as can be seen in the following demo: https://stackblitz.com/edit/react-xffwnt-ko8kc5?file=Demo.tsx But this has some limitations (e.g. can't overlay the error over old data).

As a sidenote, error handling is such an important consideration in serverside loading scenarios, yet none of the DataGrid demos take it into account. The fake useQuery function simply ignores the fact that this function in react-query can return an error. In the ideal scenario, errors can be easily displayed in-context, inside of the grid itself. Since MUI X removed the error prop, we've really had to build a lot of machinery in Toolpad to handle serverside errors. Personally I don't understand why this property was removed. I now have to wrap every grid in an error boundary + a separate overlay for row loading errors. It's quite inconvenient.

Examples šŸŒˆ

No response

Motivation šŸ”¦

No response

Search keywords: error overlay

MBilalShafi commented 9 months ago

Thank you @Janpot for raising the issue. If I recall it correctly we removed the error prop and ErrorOverlay when we removed the ErrorBoundary from within Data Grid for users to be able to use it outside the Grid. For reference, here's a related discussion and the problems around error handling reported which started this discussion.

IMHO, there was no other obvious reason for removing the error prop along with error boundary other than it being easier to be done on users' side and one less prop to be handled by grid. (@cherniavskii please correct me if I missed something). So, if there's an obvious need and pain points faced by many people, we can always bring them back.

By the way, I tried to adjust your example with the known headerHeight and a little bit of tweaking in the CSS and it seems to kind-of work without hijacking the NoRowsOverlay slot. Do you think if this could possibly be a solution/direction for similar issues?

cherniavskii commented 4 months ago

We discussed this during the weekly meeting ā€“ we'll bring back the error overlay (with a dedicated slot) and the error prop. We won't bring the ErrorBoundary back, you'll have to wrap the DataGrid with it if necessary. @Janpot How does it sound?

Janpot commented 4 months ago

We won't bring the ErrorBoundary back, you'll have to wrap the DataGrid with it if necessary.

How will that work when for instance the data is missing an id field? Will it throw an error? Or will it display in the rows area? i.e. when I have a backend error while loading the rows, I'd like to show as much as possible of the grid UI (toolbar, pagination controls, column headers) and just show an error in the area where otherwise the rows would render. In my intuition, and error with the format of the row data would go in there as well. But if the grid throws in that case, this won't be possible. Or at least, I'd like to somehow be able to display missing id error there when Toolpad is in development mode (or cell formatting error,...).

cherniavskii commented 4 months ago

How will that work when for instance the data is missing an id field? Will it throw an error? Or will it display in the rows area?

That's a good question. Yes, it will throw an error and you should be able to catch it in the error boundary: https://codesandbox.io/p/sandbox/epic-darkness-8j4hdl

Interestingly, Data Grid v5 that had the error boundary and error overlay doesn't catch it either: https://codesandbox.io/p/sandbox/wizardly-star-8hmr8q?file=%2Fsrc%2FDemo.tsx%3A12%2C22 I believe this is because the error is thrown up in the tree before Data Grid even has a chance to render anything.

when I have a backend error while loading the rows, I'd like to show as much as possible of the grid UI (toolbar, pagination controls, column headers) and just show an error in the area where otherwise the rows would render.

In Data Grid v5, the error overlay was covering the whole Data Grid: https://stackblitz.com/edit/react-8vqwuz?file=Demo.tsx,package.json,index.tsx I think we can make it configurable in v7 and have an error overlay that only covers the rows rendering container. This would only work for errors passed to the Data Grid via the error prop, and won't work if the error is thrown by the Data Grid during rendering.

I'd like to somehow be able to display missing id error there when Toolpad is in development mode

I'm not sure it's possible since the missing Id error is thrown at the top of the Data Grid tree.