[DataGrid] Add getRowId prop

[dtassone]

fix #378

[oliviertassinari]

We need to update the api pages to mention the existence of the prop, otherwise, developers won't find it.

[dtassone]

consider renaming row.id to row.rowId to avoid conflict with users' data 🤔

[oliviertassinari]

I like the updated code architecture of the PR 👍 but there is a performance regression, at least seems to be of 1 extra second with 100,000 rows. I believe it's related to the cloning behavior I have reported previously. Compare:

HEAD:
https://master--material-ui-x.netlify.app/components/data-grid/#commercial-version

PR:
https://deploy-preview-972--material-ui-x.netlify.app/components/data-grid/#commercial-version

This is the time it takes to rerender when pressing this button (the same thing with the initial render but harder to measure is isolation):

[dtassone]

Test on this branch with spread
convertRowsPropToState: 1827.47998046875 ms
convertRowsPropToState: 1723.870849609375 ms
with mutation
convertRowsPropToState: 1510.81982421875 ms

Test on master
convertRowsPropToState: 1302.665283203125 ms
convertRowsPropToState: 1035.637939453125 ms

This test was run using dev mode, react dev version, debugger open, (grid debug off)... Perf should be faster in prod

[oliviertassinari]

What should we do then?

This test was run using dev mode

A side note on this as I don't recall making this point in the past: We shouldn't make decisions based on performance in development. It can be used as a tool to nourish intuition but has little to no value compared to what the performance in production are.

[dtassone]

What should we do then?

This test was run using dev mode

A side note on this as I don't recall making this point in the past: We shouldn't make decisions based on performance in development. It can be used as a tool to nourish intuition but has little to no value compared to what the performance in production are.

We shouldn't make decisions based on performance in development.

Agree!

Basically it seems that it creates a small perf hit of around 300-500ms when you parse about 100 000 rows.
Well anyone should go server side for that amount of rows, but if we want to avoid the perf hit, then we can only clone the row when getRowId is used, which can lead to edge case issues...
If we consider that 10 000 rows is our benchmark then we have 30-50 ms extra.

[oliviertassinari]

I would personally vote for going down the edge cases direction (no clone when possible). We can easily revert. So far, we only have a small number of entry points for feeding rows and the difference can be felt when opening HEAD vs. PR (look at the time the "No rows message" is shown, it goes from a few 100 ms to over a second).

I also like the idea that we keep the same reference of the row, which can be important for developers to perform referential comparisons.

Maybe it's also a signal that having an intermediary row data structure can help (remove the need to clone)

@DanailH what do you think?

[dtassone]

We could remove having a row.id attached to the row data, so we would store ids in the state lookup key and we have an id array.
Having a row.id is easier from a coding perspective and if we remove it, it will be a quite big refactoring as we would need to pass it separately in few places but it's doable 🤔

[oliviertassinari]

We could remove having a row.id attached to the row data, so we would store ids in the state lookup key and we have an id array.
Having a row.id is easier from a coding perspective and if we remove it, it will be a quite big refactoring as we would need to pass it separately in few places but it's doable 🤔

Yeah, if we were to change the structure of the state, it might even be better to store the id into a structure close to:

interface Row {
  id?: string | number; // extracted id
  rowData?: any; // input, referential equal with object provided by the developer.
}

[dtassone]

We could remove having a row.id attached to the row data, so we would store ids in the state lookup key and we have an id array.
Having a row.id is easier from a coding perspective and if we remove it, it will be a quite big refactoring as we would need to pass it separately in few places but it's doable 🤔

Yeah, if we were to change the structure of the state, it might even be better to store the id into a structure close to:

interface Row {
  id?: string | number; // extracted id
  rowData?: any; // input, referential equal with object provided by the developer.
}

We had that before, but we won't really need it as mentioned above we can just use the key of the lookup...

[oliviertassinari]

We had that before, but we won't really need it as mentioned above we can just use the key of the lookup...

@dtassone True, I believe we have found a new use case for such structure: getRowId, and how it allows to not clone/mutate the data provided by the developer, reducing pressure on the memory and allowing developers to compare the reference (a pain I have seen popup a bunch of time in the main library, for instance, the combo box)

[dtassone]

We had that before, but we won't really need it as mentioned above we can just use the key of the lookup...

@dtassone True, I believe we have found a new use case for such structure: getRowId, and how it allows to not clone/mutate the data provided by the developer, reducing pressure on the memory and allowing developers to compare the reference (a pain I have seen popup a bunch of time in the main library, for instance, the combo box)

I agree with the idea of maintaining the same ref. But I'm not sure about re-adding the row structure as I don't like the idea of storing meta row data in this row model as it was before as it removes the composability of the component.
ie if you store the selected state on the row object and not use the useSelected hook then you have a useless field in the structure.

[oliviertassinari]

@dtassone Agree, to consider for later