iTwin / iTwinUI

A design system for building beautiful and well-working web interfaces.
https://itwin.github.io/iTwinUI/
MIT License
105 stars 38 forks source link

Table crash when using react 18 and react-query #900

Open Woyken opened 1 year ago

Woyken commented 1 year ago

Describe the bug (actual behavior)

When using react 18 rendering, and using react-query Table rendering will crash with "Maximum update depth exceeded" error.

Expected behavior

No crash and render empty table

Reproduction

More often reproduced in stackblitz https://stackblitz.com/edit/github-w9fjy7-vlsnjh?file=src%2FApp.tsx,src%2Fitwinui%2FqueryTest.tsx

Sometimes it's finicky to reproduce in codesandbox, might happen 1/10 page refreshes https://codesandbox.io/s/itwinui-react-example-forked-m5hk5x?file=/src/App.tsx

Steps to reproduce
  1. Render react with React 18
  2. useQuery from reactQuery
  3. Render Table element (does not need to use query result)

Additional information

I couldn't figure out why and how the renders are triggered in react-query to make this workflow crash iTwinUI-react is using react-table 7, maybe, just maybe this will be fixed by updating to react-table 8 https://tanstack.com/table/v8/docs/adapters/react-table

I'm pretty sure this only happens when data is provided synchronously to react-query. Happened when testing. If there is a delay before data is returned, for example with setTimeout, it seems to work, useDeferredValue seems to help too

Workaround: use useDeferredValue

const { data } = useQuery();
const deferredData = useDeferredValue(data);

return <Table data={deferredData} />
mayank99 commented 1 year ago

Thanks for reporting this @Woyken. If I had to guess, the data returned by react-query is not properly memoizable?

Since you can work around this with useDeferredValue, I have added that label. We are planning to migrate to react-table 8 in the next major release, so we can come back to this then.

@bentleyvk Do you think there is anything we can do at this moment?

bentleyvk commented 1 year ago

We had a call with Woyken, at the first sight it looked like the problem is that memoization was missing but when q.data was added to the deps list, the issue showed up again. So for now I don't have a solution, need more investigation maybe.