[DataGrid] Fix useGridSelector missing subscription in React.StrictMode
#18676
Conversation
|
Deploy preview: https://deploy-preview-18676--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: 🔺+180B(0.00%) - Total Gzip Change: 🔺+13B(0.00%) Show details for 100 more bundles (22 more not shown)@mui/x-data-grid parsed: 🔺+30B(+0.01%) gzip: 🔺+3B(0.00%) |
|
FYI @lauri865 |
|
Ugh, how did you discover this or what started triggered the symptoms in the first place? I think there might be an alternative fix to this as well based on quick testing. Will post something shortly. |
This failing test. After deleting a row in pivot mode, the row index has changed, but the aggregated cell kept showing the old aggregated value because it wasn't subscribed to the state store anymore 🙃 |
Yeah, having different behaviors in dev/prod makes life harder for sure! |
|
So, if I understand what's happening correctly, it's that And why does StrictMode cause a remount of a component to begin with in this instance?! Anyways, the fix seems appropriate, since it only ever affects StrictMode. Side note: It may make StrictMode slower, since calling |
Maybe it's called too aggressively, but that's not the issue.
The component moves to a different location in the tree. |
|
I understand as much. My point was that the first getSnapshot calls come in out of order. Why does an unmounting component call getSnapshot to begin with? And the question around StrictMode was on a slightly more abstract level. If the DOM node that the component is responsible for rendering doesn't get re-created, why restart the React component life-cycle to begin with, even in StrictMode? Like, what problem is it trying to solve? |
|
By aggressively, I meant too early in the life-cycle, not too many times, just to avoid any confusion. |
The first
Is this what happens, though? If I uncomment the second |
Hmm, this seems to work well. |
This seems to be an expected behavior: https://stackoverflow.com/a/76475288 |
This made me think – shouldn't we remove the |
Unfortunately not. Because that would run after any ref is set, and wouldn't catch state updates from callback refs. In this particular edge case it shouldn't matter, as the node should be available anyways. I would love to get rid of the whole Any other idea I've tried breaks StrictMode completely (e.g. setting up the subscription in createRefs or the callback function useState and other such ideas). The problem is always the same – unsubscribe gets called after the second component lifecycle has already started. |
|
Performance-wise, if we want to reduce allocations and function calls, it could be possible to add a differentiated export based on whether we're running in production or not (React itself does it internally). In production we don't have to care about StrictMode, and hence something as simple as this could work: https://stackblitz.com/edit/v3km56jq-3mmtkbbj?file=src%2Findex.jsx,src%2FDemo.jsx Won't affect bundle size (if anything a tiny improvement), but makes it more difficult to maintain dual tracks of course, and probably would need to remove StrictMode from testing to make sure we test production code. Just wish React would have offered an escape hatch for StrictMode for library maintainers, and not act as such a babysitter. |
|
Actually, I was reminded that double-rendering was only introduced in React 18, so we could have Similar to this: https://github.com/emotion-js/emotion/blob/49229553967b6050c92d9602eb577bdc48167e91/packages/use-insertion-effect-with-fallbacks/src/index.ts#L14 Roughly like this (the logic can be further deduplicated of course): In this case, testing can remain the same. |
This feels riskier than what we do ATM, because the |
so that it's easier to find and disable when necessary
I noticed this issue while fixing failing tests in #18348
In strict mode, when a grid cell unmounts and remounts in a different location in the DOM,
useGridSelectordoesn't subscribe to the store and displays an outdated state.Minimal reproduction example (no data grid used): https://stackblitz.com/edit/v3km56jq?file=src%2FDemo.jsx
Fix: uncomment lines 85-87