[DataGrid] Fix deferred rendering race condition

[dtassone]

fix #1784

Preview: https://deploy-preview-1807--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-rows--defer-rendering

[oliviertassinari]

This test passes on HEAD. It's as if we didn't add any tests.

[dtassone]

then let's get rid of it and keep the snap! Why do we need to waste time fighting the test framework when we can do a snap and the test is covered?

[oliviertassinari]

Because the feedback loop for a snap is 1. longer than a test and 2. provide no assertions.

[oliviertassinari]

@m4theushw Do you want to help with the test case?

[dtassone]

1. longer ie 350ms / 2sec ? => negligible in my opinion
2. It actually provides a much better assertion as it checks the full rendering of the grid.

But fine, I will fix the test if you prefer.

[oliviertassinari]

It's definitely a tradeoff. But I think that we should always come to this: snap => mainly for aesthetic/layout,

1. Say you are working on a problem where fixing one thing, breaks 3 others. Can you add an .only to run the test you care about and quickly know which test fails or pass? Or do you have to manually visual check that each instance is not broken, each time you iterate?
2. How much easier is it to introduce a regression by mistake? With a snap, we have to check that somebody didn't approve a change by mistake (sometimes, there are 100 visual diffs, hard to review them all), with a programmatic test, we see it in the git diff. You can also blame, knowing when the change happens and what was the context.

Looking closer, since it's a timing issue I'm not even sure it can be tested with the karma/jsdom test. What I would recommend is using the e2e test. It has the same runtime as the visual (so no need to "fight" with the test infra) but adds the extra benefit of indisputable assertions.

[dtassone]

1. I'm not sure I follow. You can open the story and fix the issue. You then check the other broken snap and fix them as well. In this particular case, it's the first rendering with a delay that we are testing so IMO snap is enough
2. Yes sure, I'm not saying we drop all the karma tests, I'm saying that for this case, we just take the frictionless path.

[dtassone]

It works in safari 13.2.1 where browserstack is failing.

[oliviertassinari]

You can open the story and fix the issue. You then check the other broken snap and fix them as well.

1. Yes, this is the feedback loop, it will always be the same, for all the types of tests we right. My concern is with the duration of the feedback loop and how it can sustain a test codebase of x10 moe tests on the data grid. I don't want to have to wait 6 minutes for the CI to run to know if my latest batch of changes is correct. It feels daunting to visually check that 20 snaps are correct when working on one feature in which fixing on test often break another.

For Safari, I have pushed on the e2e test direction in #1831 because we depend on having the timing right. We need to be async, we can't mock. But it doesn't always fail :/. It's failing a lot more reliabily than on karma, but not at 100% 🤷‍♂️.

[oliviertassinari]

I had a look at the issue, It's really interesting. What I could identify is that React handling the updates in two different threads, with no guarantee in the order it would resolve. So we can either solve it by running the update that we care about first (containerSizes):

diff --git a/packages/grid/_modules_/grid/hooks/root/useGridContainerProps.ts b/packages/grid/_modules_/grid/hooks/root/useGridContainerProps.ts
index 11f40def..bce03312 100644
--- a/packages/grid/_modules_/grid/hooks/root/useGridContainerProps.ts
+++ b/packages/grid/_modules_/grid/hooks/root/useGridContainerProps.ts
@@ -242,20 +242,19 @@ export const useGridContainerProps = (
       return;
     }

+    const containerState = getContainerProps(rowsCount, viewportSizes, scrollBar);
+
     updateStateIfChanged(
-      (state) => state.scrollBar !== scrollBar,
-      (state) => ({ ...state, scrollBar }),
+      (state) => !isDeepEqual(state.containerSizes, containerState),
+      (state) => ({ ...state, containerSizes: containerState }),
     );
-
     updateStateIfChanged(
       (state) => state.viewportSizes !== viewportSizes,
       (state) => ({ ...state, viewportSizes }),
     );
-
-    const containerState = getContainerProps(rowsCount, viewportSizes, scrollBar);
     updateStateIfChanged(
-      (state) => !isDeepEqual(state.containerSizes, containerState),
-      (state) => ({ ...state, containerSizes: containerState }),
+      (state) => state.scrollBar !== scrollBar,
+      (state) => ({ ...state, scrollBar }),
     );
   }, [
     getContainerProps,

or avoid any memorization and read from the parent render scope:

diff --git a/packages/grid/_modules_/grid/hooks/root/useGridContainerProps.ts b/packages/grid/_modules_/grid/hooks/root/useGridContainerProps.ts
index 11f40def..27bd0377 100644
--- a/packages/grid/_modules_/grid/hooks/root/useGridContainerProps.ts
+++ b/packages/grid/_modules_/grid/hooks/root/useGridContainerProps.ts
@@ -12,6 +12,7 @@ import { gridColumnsTotalWidthSelector } from '../features/columns/gridColumnsSe
 import { GridState } from '../features/core/gridState';
 import { useGridSelector } from '../features/core/useGridSelector';
 import { useGridState } from '../features/core/useGridState';
+import { useEventCallback } from '../../utils/material-ui-utils';
 import { gridDensityRowHeightSelector } from '../features/density/densitySelector';
 import { visibleGridRowCountSelector } from '../features/filter/gridFilterSelector';
 import { gridPaginationSelector } from '../features/pagination/gridPaginationSelector';
@@ -232,7 +233,7 @@ export const useGridContainerProps = (
     [forceUpdate, setGridState],
   );

-  const refreshContainerSizes = React.useCallback(() => {
+  const refreshContainerSizes = useEventCallback(() => {
     logger.debug('Refreshing container sizes');
     const rowsCount = getVirtualRowCount();
     const scrollBar = getScrollBar(rowsCount);
@@ -257,14 +258,7 @@ export const useGridContainerProps = (
       (state) => !isDeepEqual(state.containerSizes, containerState),
       (state) => ({ ...state, containerSizes: containerState }),
     );
-  }, [
-    getContainerProps,
-    getScrollBar,
-    getViewport,
-    getVirtualRowCount,
-    logger,
-    updateStateIfChanged,
-  ]);
+  });

   React.useEffect(() => {
     refreshContainerSizes();

It fixes the issue in both cases.

[dtassone]

the first diff fail the browserstack safari test, how did you make it work?

[oliviertassinari]

@dtassone What's the next step on this PR?

What I could notice:

1. We need to rebase, the PR contains changes (prettier) that are off-topic and harms the reviewability of the PR.
2. The programmatic test is not failing on master. We could remove it, it would be the same.
3. The visual test is IMHO harmful, I would vote for removing Snap from the story's name. I have introduced visual snapshots to test visual layouts CSS. I think that we should ban all functional test usage of visual screenshots. (Also one of the values of the Snap suffix tradeoff, to make it easier to spot offenders' stories).
4. We have a more reliable test in [DataGrid] Fix React concurrent updates #1831 we can cherry-pick here.
5. The fix we have here might not continue working in the future or have side effects, if needed, in the future, we could leverage the fix closer to the root cause in [DataGrid] Fix React concurrent updates #1831.
6. I didn't look at why, but if I had to bet, the test in this PR is failing in Safari for the same reason the new story is failing too when switching between two pages: the setTimeout, 0 resolves before setTimeout, 1. Which is fine, 0, 1, is meant to say the minimum amount of time before the value resolves 0, can resolve after 1, leading to rows={[]}.

• We can fix this with:

import { XGrid } from "@material-ui/x-grid";
import { useEffect, useState } from "react";

const columns = [{ field: "id", headerName: "Id", width: 100 }];

export default function XGridDemo() {
  const [rows, setRows] = useState([]);

  useEffect(() => {
    setTimeout(() => setRows([{ id: 1 }, { id: 2 }]), 0);
  }, []);

  console.log(rows);
  return <XGrid autoHeight columns={columns} rows={rows} />;
}

It's more reliable. It still fails on HEAD https://codesandbox.io/s/kind-hopper-634sb?file=/src/demo.js, and yet the rows prop is no longer empty when switching between two stories. See the storybook preview of #1831.