[DataGrid] Spotting perf issues

[dtassone]

This PR aims to

• fix some perf bottleneck and refactor the selection.
• Also fix an issue with the sorting

[oliviertassinari]

The tests surface a regression, sorting is no longer working on the first click. You can reproduce on https://deploy-preview-501--material-ui-x.netlify.app/components/data-grid/rows/#basic-sorting. It's actually great that we are doing this refactorization, each bug is an opportunity to add a new test case.

[DanailH]

The tests surface a regression, sorting is no longer working on the first click. You can reproduce on https://deploy-preview-501--material-ui-x.netlify.app/components/data-grid/rows/#basic-sorting. It's actually great that we are doing this refactorization, each bug is an opportunity to add a new test case.

I had the same problem after I removed the React.memo from the column-header-item. Hope that helps :D

[oliviertassinari]

fix an issue with the sorting

Is the bug something we can/should add a test case for?

We already have tests for sorting. It was just small display issue.

[dtassone]

Any idea @oliviertassinari why karma is not happy? Or is it just personal 😛

[oliviertassinari]

why karma is not happy?

I'm looking at it. Something prevents the test to complete it.

[oliviertassinari]

I could pinpoint the test that fails. it's:

https://github.com/mui-org/material-ui-x/blob/f1835be6837097cbdf430b1fe99f938af77fe5b4/packages/grid/x-grid/src/XGrid.test.tsx#L192

It freezes on the selection click event.

[oliviertassinari]

There are two problems:

1. The click event is no longer act-synchronously handled, hence the assertion fails. This is because we now relies on a RAF that React's act can't handle. So we miss a await raf().

diff --git a/packages/grid/x-grid/src/XGrid.test.tsx b/packages/grid/x-grid/src/XGrid.test.tsx
index b5da9b88..a7e9c0f4 100644
--- a/packages/grid/x-grid/src/XGrid.test.tsx
+++ b/packages/grid/x-grid/src/XGrid.test.tsx
@@ -189,7 +189,7 @@ describe('<XGrid />', () => {
   });

   describe('prop: checkboxSelection', () => {
-    it('should check and uncheck when double clicking the row', async () => {
+    it.only('should check and uncheck when double clicking the row', async () => {
       render(
         <div style={{ width: 300, height: 300 }}>
           <XGrid
@@ -213,10 +213,12 @@ describe('<XGrid />', () => {
       expect(checkbox).to.have.property('checked', false);

       fireEvent.click(screen.getByRole('cell', { name: 'Nike' }));
+      await raf();
       expect(row).to.have.class('Mui-selected');
       expect(checkbox).to.have.property('checked', true);

       fireEvent.click(screen.getByRole('cell', { name: 'Nike' }));
+      await raf();
       expect(row).to.not.have.class('Mui-selected');
       expect(checkbox).to.have.property('checked', false);
     });

2. The second is really weird and worrying. It's the first time I see it, I struggle to reproduce it in a clean sheet. The expect(row).to.have.class('Mui-selected'); assertion crashes. On the other hand, this correctly fail. @eps1lon have you ever seen something like this? :

diff --git a/packages/grid/x-grid/src/XGrid.test.tsx b/packages/grid/x-grid/src/XGrid.test.tsx
index b5da9b88..5c1f4a08 100644
--- a/packages/grid/x-grid/src/XGrid.test.tsx
+++ b/packages/grid/x-grid/src/XGrid.test.tsx
@@ -189,7 +189,7 @@ describe('<XGrid />', () => {
   });

   describe('prop: checkboxSelection', () => {
-    it('should check and uncheck when double clicking the row', async () => {
+    it.only('should check and uncheck when double clicking the row', async () => {
       render(
         <div style={{ width: 300, height: 300 }}>
           <XGrid
@@ -213,11 +213,11 @@ describe('<XGrid />', () => {
       expect(checkbox).to.have.property('checked', false);

       fireEvent.click(screen.getByRole('cell', { name: 'Nike' }));
-      expect(row).to.have.class('Mui-selected');
+      expect(row!.classList.contains('Mui-selected')).to.equal(true);
       expect(checkbox).to.have.property('checked', true);

       fireEvent.click(screen.getByRole('cell', { name: 'Nike' }));
-      expect(row).to.not.have.class('Mui-selected');
+      expect(row!.classList.contains('Mui-selected')).to.equal(false);
       expect(checkbox).to.have.property('checked', false);
     });
   });

[oliviertassinari]

.have.class's implementation is straightforward, so maybe it's the serialization of the DOM node to a string that crashes.

[oliviertassinari]

We can investigate more if it reproduces more often.

[eps1lon]

The click event is no longer act-synchronously handled, hence the assertion fails. This is because we now relies on a RAF that

Why would it rely on requestAnimationFrame?

[oliviertassinari]

Why would it rely on requestAnimationFrame?

@eps1lon I doubt that the issue with the test is related to the RAF. I suspect that the DOM structure of the row has something that chai-dom doesn't like, but it's only a hypothesis. For why a RAF we can chat more about it in #507.