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.52k stars 1.31k forks source link

[data grid] infinite loop occurs under certain circumstances #13230

Closed layerok closed 5 months ago

layerok commented 5 months ago

Steps to reproduce

Link to live example: https://codesandbox.io/p/devbox/frosty-faraday-lt52lc?file=%2Fsrc%2FApp.tsx%3A135%2C24

Steps:

  1. You don't have to do anything, just open the console and see that the error is thrown indefinitely, don't worry I limited it to maximum of 50 cycles so that your browser doesn't crush

Current behavior

Infinite loop

Expected behavior

No infinite loop

Context

I want to...

  1. calculate page size based on data grid dimensions.
  2. preserve the pagination model in the URL.

I have successfully completed everything listed above. But there is one problem that occurs under certain circumstances. Infinite loop. The circumstances are as follows.

If all of the circumstances are true, the infinite loop is guaranteed. If one of the circumstances isn't true, the infinite loop doesn't occur.

This what I understood what is happening

  1. An error is thrown while rendering the data grid
  2. The error is caught by the react-router error boundary
  3. The error boundary is rendered.
  4. The viewportInnerSizeChange event is emitted in useEffect in the useGridDimensions hook
  5. Search params are updated in my custom viewportInnerSizeChange listener
  6. The error boundary is unmounted because the location was changed due to the search params change
  7. The data grid renders again
  8. All of above steps are repeated indefinitely

What's weird is why the viewportInnerSizeChange event is emitted after the error boundary is rendered. I'm guessing this useEffect might be causing the error.

  useEnhancedEffect(() => {
    if (savedSize) {
      updateDimensions();
      apiRef.current.publishEvent('debouncedResize', rootDimensionsRef.current!);
    }
  }, [apiRef, savedSize, updateDimensions]);

Your environment

No response

Search keywords: react router infinite loop loader viewportInnerSizeChange navigate Order ID: 74777

michelengelen commented 5 months ago

Hey @layerok ... thanks for opening this issue. I see that you already came up with a solution and opened a PR. I did add some reviewers to that so it gets picked up by the team. 👍🏼

romgrk commented 5 months ago

IIUC, to trigger this issue, there needs to be an error thrown from inside the grid rendering code, which in this case is user code, right?

layerok commented 5 months ago

Yes, in this case the error came from user code, but that doesn't matter. The error could have come from the library code. For example, an error is thrown if any translation string is missing.

romgrk commented 5 months ago

I think the issue is the error, not the infinite loop. There shouldn't be an error thrown from the translation code, everything should be complete. I would be ok with adding an error boundary to catch the error and fail gracefully, but I don't think we should start adding code to the rest of the logic because "anything might throw". It just complicates the logic too much for what it brings.

layerok commented 5 months ago

First of all, "everything should be complete" for me is a very optimistic assumtion in this situation. If this assumtion is not true, the user's browser will crush. I don't want that. For example, my browser crushed because an unexpected error was thrown from inside library code.

Secondly, I don't think there is only one cause to this issue. This issue won't reproduce if one of 3 conditions is absent. I don't know for sure if this is mui or react-router bug. It might be a bug in react-router or in both libraries simultaneously. I created the issue in mui, because I had to start somewhere.

You said that you would be ok to wrap datagrid in an error boundary to fail gracefully. I'm not sure if this will help, react-router already wraps every route in an error boundary, but it does not help.

Right now I'm trying to create a minimal reproducible example to understand what the problem is. I'm creating my custom react-router and datagrid implementations, removing everything that isn't related to the problem.

layerok commented 5 months ago

I found one interesting detail. An error that occurs while rendering a cell does not result in the infinite loop, unlike errors within the header and header filter cells.

layerok commented 5 months ago

ok, I figured it out, it turns out that calling the router.navigate function in useEffect schedules update in a microtask that cannot be cancelled. What I mean is when you are doing this

useEffect(() => {
  router.navigate(...)
}, []);

You are essentially doing this

useEffect(() => {
  setState({...});
  // react-router resolves loaders
  Promise.resolve().then(() => {
    // state update in a microtask
    setState({...});
  })
}, [])

To fix the infinite loop we need to somehow cancel this microtask. Unfortunately there is no way to cancel pending navigation in react-router. But we can workaround this problem by wrapping router.navigate call in setTimeout. This way we will be able to abort the navigation before it even starts.

useEffect(() => {
  let timeout = setTimeout(() => {
      router.navigate(...);
  })
  return () => clearInterval(timeout);
}, [])

It also would be nice if MUI didn't emit events inside layout or passive effect, thus this problem wouldn't exist at all. But I guess this will be massive rework of the library.

layerok commented 5 months ago

Finally, I've created a minimally reproducible example as possible Now it all makes sense to me. Why removing loaders can fix the infinite loop. Why react-router completed navigation while error boundary was rendering.

That's roughly what's going on

  1. HomePage first renders
  2. NoSsr renders but doesn't render RenderSomethingThatThrows immediately
  3. HomePage layout effects run
  4. router starts navigation, firing first setState
  5. HomePage rerenders
  6. NoSsr renders the RenderSomethingThatThrows component
  7. The error is caught by an error boundary
  8. layout effects effect run
  9. router completes navigation in the microtask, after waiting for loaders to resolve, firing second setState
  10. All about steps from step 5 are repeated indefinitely
const HomePage = () => {
  const navigate = useNavigate();
  useEffect(() => {
    //const timeout = setTimeout(() => {
      navigate({
        search: "?page=2"
      })
    //})
    return () => {
      //clearTimeout(timeout)
    }
  }, [navigate]);

  return (
    <NoSsr>
      <RenderSomethingThatThrows/>
    </NoSsr>
  );
}

const router = createBrowserRouter([
  {
    path: "/",
    element: <HomePage />,
    loader: async () => {
      // remove this loader to prevent the infinite loop
      return null;
    },
  },
]);

function App() {
  return <RouterProvider router={router} />;
}
layerok commented 5 months ago

this is not a MUI issue

github-actions[bot] commented 5 months ago

:warning: This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

@layerok: How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.