[DataGrid] Reset sortedRows state on prop change

[dtassone]

Fix #575
Fix #571

[oliviertassinari]

sortedRows looks like an internal state that should be private. It's fully derived from the input rows and the sort configuration provided by users. Developers' changes to the state are overridden by the grid. So I don't think that any tests should assert it.

On a side note, I was surprised to see this state exist. I was assuming that we would have a "input row" state, as well as a "transformed row" state, the transformed one being computed from grouping, filtering, sorting, etc. It seems that the challenge is to make sure that each transformations is applied in the right order. I guess it can be resolved with a priority value.

[oliviertassinari]

Something is off, it jumps:

It renders twice, shouldn't it render once?

import * as React from 'react';
import { DataGrid } from '@material-ui/data-grid';

const TestCase = () => {
  const [rows, setRows] = React.useState([
    {
      id: 0,
      brand: 'Nike',
    },
    {
      id: 1,
      brand: 'Adidas',
    },
    {
      id: 2,
      brand: 'Puma',
    },
  ]);

  return (
    <div style={{ width: 300, height: 300 }}>
      <button onClick={() => {
        setRows([
          {
            id: 3,
            brand: 'Asics',
          },
          {
            id: 4,
            brand: 'RedBull',
          },
          {
            id: 5,
            brand: 'Hugo',
          },
        ]);
      }}>Swap</button>
      <DataGrid
        columns={[{ field: 'brand', width: 100 }]}
        rows={rows}
        sortModel={[
        {
          field: 'brand',
          sort: 'asc',
        },
      ]}
      />
    </div>
  );
};

export default TestCase;

[oliviertassinari]

If we sorted the rows directly, would it solve #599 (comment)?

[dtassone]

At least we fixed the reported bug and we have a solid test in place, so why not defer a better solution to later. I suspect it will require a refactoring of the state. There is an issue with the control flow somewhere, it could be imperative calls that shouldn't be here, it could be missing dependencies, a desynchronization, no idea :).

I will fix it ;)

[dtassone]

Fixed

[oliviertassinari]

At least we fixed the reported bug and we have a solid test in place, so why not defer a better solution to later. I suspect it will require a refactoring of the state. There is an issue with the control flow somewhere, it could be imperative calls that shouldn't be here, it could be missing dependencies, a desynchronization, no idea :).

[oliviertassinari]

Nice :)